Repository: tetratelabs/wazero
Pull request #1473 (2023-05-16 04:47:00)

Require max size for shared memory
So sorry, I completely forgot to address this issue from @ncruces 

https://github.com/tetratelabs/wazero/pull/1460#discussion_r1193557372

anuraaga: I've been seeing the windows tests fail with OOM sometimes but not often - just checking if that's been seen before. If not, it must be because of my PR and I'll look into it
codefromthecrypt: opened https://github.com/tetratelabs/wazero/issues/1476 on the flake. thanks for reporting

Patch:
diff --git a/internal/platform/file.go b/internal/platform/file.go
index 4a0e9cb0..eed20ef7 100644
--- a/internal/platform/file.go
+++ b/internal/platform/file.go
@@ -42,17 +42,16 @@ type File interface {
 	//   - Some implementations implement this with a cached call to Stat.
 	Ino() (uint64, syscall.Errno)
 
-	// IsNonblock returns true if SetNonblock was successfully enabled on this
-	// file.
+	// IsNonblock returns true if the file was opened with O_NONBLOCK, or
+	// SetNonblock was successfully enabled on this file.
 	//
 	// # Notes
 	//
-	//   - This may not match the underlying state of the file descriptor if it
-	//     was opened (OpenFile) in non-blocking mode.
+	//   - This might not match the underlying state of the file descriptor if
+	//     the file was not opened via OpenFile.
 	IsNonblock() bool
-	// ^-- TODO: We should be able to cache the open flag and remove this note.
 
-	// SetNonblock toggles the non-blocking mode of this file.
+	// SetNonblock toggles the non-blocking mode (O_NONBLOCK) of this file.
 	//
 	// # Errors
 	//
@@ -62,20 +61,20 @@ type File interface {
 	//
 	// # Notes
 	//
-	//   - This is like syscall.SetNonblock and `fcntl` with `O_NONBLOCK` in
+	//   - This is like syscall.SetNonblock and `fcntl` with O_NONBLOCK in
 	//     POSIX. See https://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
 	SetNonblock(enable bool) syscall.Errno
 
-	// IsAppend returns true if SetAppend was successfully enabled on this file.
+	// IsAppend returns true if the file was opened with syscall.O_APPEND, or
+	// SetAppend was successfully enabled on this file.
 	//
 	// # Notes
 	//
 	//   - This might not match the underlying state of the file descriptor if
-	//     it was opened (OpenFile) in append mode.
+	//     the file was not opened via OpenFile.
 	IsAppend() bool
-	// ^-- TODO: We should be able to cache the open flag and remove this note.
 
-	// SetAppend toggles the append mode of this file.
+	// SetAppend toggles the append mode (syscall.O_APPEND) of this file.
 	//
 	// # Errors
 	//
diff --git a/internal/wasm/binary/memory.go b/internal/wasm/binary/memory.go
index ec8b37f3..265361ac 100644
--- a/internal/wasm/binary/memory.go
+++ b/internal/wasm/binary/memory.go
@@ -23,8 +23,15 @@ func decodeMemory(
 		return nil, err
 	}
 
-	if shared && !enabledFeatures.IsEnabled(experimental.CoreFeaturesThreads) {
-		return nil, fmt.Errorf("shared memory requested but threads feature not enabled")
+	if shared {
+		if !enabledFeatures.IsEnabled(experimental.CoreFeaturesThreads) {
+			return nil, fmt.Errorf("shared memory requested but threads feature not enabled")
+		}
+		// This restriction may be lifted in the future.
+		// https://webassembly.github.io/threads/core/binary/types.html#memory-types
+		if maxP == nil {
+			return nil, fmt.Errorf("shared memory requires a maximum size to be specified")
+		}
 	}
 
 	min, capacity, max := memorySizer(min, maxP)


Test patch:
diff --git a/internal/integration_test/engine/threads_test.go b/internal/integration_test/engine/threads_test.go
index 7fd82202..61e51dd7 100644
--- a/internal/integration_test/engine/threads_test.go
+++ b/internal/integration_test/engine/threads_test.go
@@ -57,6 +57,10 @@ func TestThreadsInterpreter(t *testing.T) {
 }
 
 func incrementGuardedByMutex(t *testing.T, r wazero.Runtime) {
+	P := 8               // max count of goroutines
+	if testing.Short() { // Adjust down if `-test.short`
+		P = 4
+	}
 	tests := []struct {
 		fn string
 	}{
@@ -88,7 +92,7 @@ func incrementGuardedByMutex(t *testing.T, r wazero.Runtime) {
 			mod, err := r.Instantiate(testCtx, mutexWasm)
 			require.NoError(t, err)
 
-			hammer.NewHammer(t, 8, 30000).Run(func(name string) {
+			hammer.NewHammer(t, P, 30000).Run(func(name string) {
 				_, err := mod.ExportedFunction(tt.fn).Call(testCtx)
 				require.NoError(t, err)
 			}, func() {})
@@ -96,47 +100,51 @@ func incrementGuardedByMutex(t *testing.T, r wazero.Runtime) {
 			// Cheat that LE encoding can read both 32 and 64 bits
 			res, ok := mod.Memory().ReadUint32Le(8)
 			require.True(t, ok)
-			require.Equal(t, uint32(8*30000), res)
+			require.Equal(t, uint32(P*30000), res)
 		})
 	}
 }
 
 func atomicAdd(t *testing.T, r wazero.Runtime) {
+	P := 8               // max count of goroutines
+	if testing.Short() { // Adjust down if `-test.short`
+		P = 4
+	}
 	tests := []struct {
 		fn  string
-		exp uint32
+		exp int
 	}{
 		{
 			fn:  "run32",
-			exp: 8 * 30000,
+			exp: P * 30000,
 		},
 		{
 			fn:  "run64",
-			exp: 8 * 30000,
+			exp: P * 30000,
 		},
 		{
 			fn: "run32_8",
 			// Overflows
-			exp: (8 * 30000) % (1 << 8),
+			exp: (P * 30000) % (1 << 8),
 		},
 		{
 			fn: "run32_16",
 			// Overflows
-			exp: (8 * 30000) % (1 << 16),
+			exp: (P * 30000) % (1 << 16),
 		},
 		{
 			fn: "run64_8",
 			// Overflows
-			exp: (8 * 30000) % (1 << 8),
+			exp: (P * 30000) % (1 << 8),
 		},
 		{
 			fn: "run64_16",
 			// Overflows
-			exp: (8 * 30000) % (1 << 16),
+			exp: (P * 30000) % (1 << 16),
 		},
 		{
 			fn:  "run64_32",
-			exp: 8 * 30000,
+			exp: P * 30000,
 		},
 	}
 	for _, tc := range tests {
@@ -145,7 +153,7 @@ func atomicAdd(t *testing.T, r wazero.Runtime) {
 			mod, err := r.Instantiate(testCtx, addWasm)
 			require.NoError(t, err)
 
-			hammer.NewHammer(t, 8, 30000).Run(func(name string) {
+			hammer.NewHammer(t, P, 30000).Run(func(name string) {
 				_, err := mod.ExportedFunction(tt.fn).Call(testCtx)
 				require.NoError(t, err)
 			}, func() {})
@@ -153,47 +161,51 @@ func atomicAdd(t *testing.T, r wazero.Runtime) {
 			// Cheat that LE encoding can read both 32 and 64 bits
 			res, ok := mod.Memory().ReadUint32Le(0)
 			require.True(t, ok)
-			require.Equal(t, tt.exp, res)
+			require.Equal(t, uint32(tt.exp), res)
 		})
 	}
 }
 
 func atomicSub(t *testing.T, r wazero.Runtime) {
+	P := 8               // max count of goroutines
+	if testing.Short() { // Adjust down if `-test.short`
+		P = 4
+	}
 	tests := []struct {
 		fn  string
-		exp int32
+		exp int
 	}{
 		{
 			fn:  "run32",
-			exp: -(8 * 30000),
+			exp: -(P * 30000),
 		},
 		{
 			fn:  "run64",
-			exp: -(8 * 30000),
+			exp: -(P * 30000),
 		},
 		{
 			fn: "run32_8",
 			// Overflows
-			exp: (1 << 8) - ((8 * 30000) % (1 << 8)),
+			exp: (1 << 8) - ((P * 30000) % (1 << 8)),
 		},
 		{
 			fn: "run32_16",
 			// Overflows
-			exp: (1 << 16) - ((8 * 30000) % (1 << 16)),
+			exp: (1 << 16) - ((P * 30000) % (1 << 16)),
 		},
 		{
 			fn: "run64_8",
 			// Overflows
-			exp: (1 << 8) - ((8 * 30000) % (1 << 8)),
+			exp: (1 << 8) - ((P * 30000) % (1 << 8)),
 		},
 		{
 			fn: "run64_16",
 			// Overflows
-			exp: (1 << 16) - ((8 * 30000) % (1 << 16)),
+			exp: (1 << 16) - ((P * 30000) % (1 << 16)),
 		},
 		{
 			fn:  "run64_32",
-			exp: -(8 * 30000),
+			exp: -(P * 30000),
 		},
 	}
 	for _, tc := range tests {
@@ -202,7 +214,7 @@ func atomicSub(t *testing.T, r wazero.Runtime) {
 			mod, err := r.Instantiate(testCtx, subWasm)
 			require.NoError(t, err)
 
-			hammer.NewHammer(t, 8, 30000).Run(func(name string) {
+			hammer.NewHammer(t, P, 30000).Run(func(name string) {
 				_, err := mod.ExportedFunction(tt.fn).Call(testCtx)
 				require.NoError(t, err)
 			}, func() {})
@@ -210,12 +222,16 @@ func atomicSub(t *testing.T, r wazero.Runtime) {
 			// Cheat that LE encoding can read both 32 and 64 bits
 			res, ok := mod.Memory().ReadUint32Le(0)
 			require.True(t, ok)
-			require.Equal(t, tt.exp, int32(res))
+			require.Equal(t, int32(tt.exp), int32(res))
 		})
 	}
 }
 
 func atomicXor(t *testing.T, r wazero.Runtime) {
+	P := 8               // max count of goroutines
+	if testing.Short() { // Adjust down if `-test.short`
+		P = 4
+	}
 	tests := []struct {
 		fn string
 	}{
@@ -249,7 +265,7 @@ func atomicXor(t *testing.T, r wazero.Runtime) {
 
 			mod.Memory().WriteUint32Le(0, 12345)
 
-			hammer.NewHammer(t, 8, 30000).Run(func(name string) {
+			hammer.NewHammer(t, P, 30000).Run(func(name string) {
 				_, err := mod.ExportedFunction(tt.fn).Call(testCtx)
 				require.NoError(t, err)
 			}, func() {})
diff --git a/internal/wasm/binary/memory_test.go b/internal/wasm/binary/memory_test.go
index d4dc1ef4..ce26a39b 100644
--- a/internal/wasm/binary/memory_test.go
+++ b/internal/wasm/binary/memory_test.go
@@ -195,9 +195,10 @@ func TestDecodeMemoryType_Errors(t *testing.T) {
 	max := wasm.MemoryLimitPages
 
 	tests := []struct {
-		name        string
-		input       []byte
-		expectedErr string
+		name           string
+		input          []byte
+		threadsEnabled bool
+		expectedErr    string
 	}{
 		{
 			name:        "max < min",
@@ -219,15 +220,25 @@ func TestDecodeMemoryType_Errors(t *testing.T) {
 			input:       []byte{0x2, 0, 0x80, 0x80, 0x4},
 			expectedErr: "shared memory requested but threads feature not enabled",
 		},
+		{
+			name:           "shared but no max",
+			input:          []byte{0x2, 0, 0x80, 0x80, 0x4},
+			threadsEnabled: true,
+			expectedErr:    "shared memory requires a maximum size to be specified",
+		},
 	}
 
 	for _, tt := range tests {
 		tc := tt
 
 		t.Run(tc.name, func(t *testing.T) {
-			// Allow test to work if threads is ever added to default features by explicitly removing threads features
 			features := api.CoreFeaturesV2
-			features = features.SetEnabled(experimental.CoreFeaturesThreads, false)
+			if tc.threadsEnabled {
+				features = features.SetEnabled(experimental.CoreFeaturesThreads, true)
+			} else {
+				// Allow test to work if threads is ever added to default features by explicitly removing threads features
+				features = features.SetEnabled(experimental.CoreFeaturesThreads, false)
+			}
 			_, err := decodeMemory(bytes.NewReader(tc.input), features, newMemorySizer(max, false), max)
 			require.EqualError(t, err, tc.expectedErr)
 		})