Repository: tetratelabs/wazero
Pull request #1946 (2024-01-23 23:21:46)

threads: don't use mmap for zero memory
Also makes sure mmapped buffer is used for munmap, not sliced buffer. Technically sliced buffer is ok, but not if it's still empty `b[:0]` it seems.


Patch:
diff --git a/internal/wasm/memory.go b/internal/wasm/memory.go
index 506b6182..3a2c0ff4 100644
--- a/internal/wasm/memory.go
+++ b/internal/wasm/memory.go
@@ -59,7 +59,7 @@ type MemoryInstance struct {
 	// with a fixed weight of 1 and no spurious notifications.
 	waiters sync.Map
 
-	mmappedMemory bool
+	mmappedBuffer []byte
 	closed        bool
 }
 
@@ -70,7 +70,7 @@ func NewMemoryInstance(memSec *Memory) *MemoryInstance {
 
 	var buffer []byte
 	var cap uint32
-	mmappedMemory := false
+	var mmappedBuffer []byte
 	if memSec.IsShared {
 		// Memory accesses can happen at the same time that memory is resized, meaning
 		// we cannot have the memory base move during operation. mmap allows allocating memory virtually so
@@ -78,13 +78,13 @@ func NewMemoryInstance(memSec *Memory) *MemoryInstance {
 		// all threads implementations are effectively expected to use mmap for shared memory.
 		max := MemoryPagesToBytesNum(memSec.Max)
 		var b []byte
-		if platform.MmapSupported {
+		if platform.MmapSupported && max > 0 {
 			var err error
 			b, err = platform.MmapMemory(int(max))
 			if err != nil {
 				panic(fmt.Errorf("unable to mmap memory: %w", err))
 			}
-			mmappedMemory = true
+			mmappedBuffer = b
 		} else {
 			// mmap not supported so we just preallocate a normal buffer. This will often be large, i.e. ~4GB,
 			// and likely isn't practical, but interpreter usage should be rare and the Wasm binary can be
@@ -104,12 +104,12 @@ func NewMemoryInstance(memSec *Memory) *MemoryInstance {
 		Cap:           cap,
 		Max:           memSec.Max,
 		Shared:        memSec.IsShared,
-		mmappedMemory: mmappedMemory,
+		mmappedBuffer: mmappedBuffer,
 	}
 }
 
 func (m *MemoryInstance) Close() error {
-	if !m.mmappedMemory {
+	if m.mmappedBuffer == nil {
 		// No need to release anything for non-mmapped memory.
 		return nil
 	}
@@ -121,7 +121,7 @@ func (m *MemoryInstance) Close() error {
 		return nil
 	}
 	m.closed = true
-	return platform.MunmapCodeSegment(m.Buffer)
+	return platform.MunmapCodeSegment(m.mmappedBuffer)
 }
 
 // Definition implements the same method as documented on api.Memory.


Test patch:
diff --git a/internal/wasm/memory_test.go b/internal/wasm/memory_test.go
index e37c5ba4..3d975003 100644
--- a/internal/wasm/memory_test.go
+++ b/internal/wasm/memory_test.go
@@ -799,6 +799,41 @@ func BenchmarkWriteString(b *testing.B) {
 	}
 }
 
+func TestNewMemoryInstance_Shared(t *testing.T) {
+	tests := []struct {
+		name string
+		mem  *Memory
+		mmap bool
+	}{
+		{
+			name: "min 0, max 1",
+			mem:  &Memory{Min: 0, Max: 1, IsMaxEncoded: true, IsShared: true},
+			mmap: true,
+		},
+		{
+			name: "min 0, max 0",
+			mem:  &Memory{Min: 0, Max: 0, IsMaxEncoded: true, IsShared: true},
+			mmap: false, // Cannot mmap buffer of 0 size
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			m := NewMemoryInstance(tc.mem)
+			defer m.Close()
+			require.Equal(t, tc.mem.Min, m.Min)
+			require.Equal(t, tc.mem.Max, m.Max)
+			require.True(t, m.Shared)
+			if tc.mmap {
+				require.NotNil(t, m.mmappedBuffer)
+			} else {
+				require.Nil(t, m.mmappedBuffer)
+			}
+		})
+	}
+}
+
 func TestMemoryInstance_WaitNotifyOnce(t *testing.T) {
 	reader := func(mem *MemoryInstance, offset uint32) uint32 {
 		val, _ := mem.ReadUint32Le(offset)