Repository: tetratelabs/wazero
Pull request #1755 (2023-10-04 23:03:42)

Removes unnecessary mutex on Table/Memory Instance
This was _somehow_ introduced in the past, but the
reality is that the mutex only used during Grow doesn't
solve any concurrent issue at all and still could result in
crash when the module was accessed incorrectly concurrently.
The proper synchronization mechanism must be provided by
threads proposal, and this mutex usage can be removed.


Patch:
diff --git a/internal/engine/compiler/engine.go b/internal/engine/compiler/engine.go
index 8fc9d52b..d4e8dcd0 100644
--- a/internal/engine/compiler/engine.go
+++ b/internal/engine/compiler/engine.go
@@ -404,7 +404,7 @@ const (
 	dataInstanceStructSize = 24
 
 	// Consts for wasm.ElementInstance.
-	elementInstanceStructSize = 32
+	elementInstanceStructSize = 24
 
 	// pointerSizeLog2 satisfies: 1 << pointerSizeLog2 = sizeOf(uintptr)
 	pointerSizeLog2 = 3
diff --git a/internal/engine/interpreter/interpreter.go b/internal/engine/interpreter/interpreter.go
index 9adb6afe..d47e7c31 100644
--- a/internal/engine/interpreter/interpreter.go
+++ b/internal/engine/interpreter/interpreter.go
@@ -1653,15 +1653,15 @@ func (ce *callEngine) callNativeFunc(ctx context.Context, m *wasm.ModuleInstance
 			inElementOffset := ce.popValue()
 			inTableOffset := ce.popValue()
 			table := tables[op.U2]
-			if inElementOffset+copySize > uint64(len(elementInstance.References)) ||
+			if inElementOffset+copySize > uint64(len(elementInstance)) ||
 				inTableOffset+copySize > uint64(len(table.References)) {
 				panic(wasmruntime.ErrRuntimeInvalidTableAccess)
 			} else if copySize != 0 {
-				copy(table.References[inTableOffset:inTableOffset+copySize], elementInstance.References[inElementOffset:])
+				copy(table.References[inTableOffset:inTableOffset+copySize], elementInstance[inElementOffset:])
 			}
 			frame.pc++
 		case wazeroir.OperationKindElemDrop:
-			elementInstances[op.U1].References = nil
+			elementInstances[op.U1] = nil
 			frame.pc++
 		case wazeroir.OperationKindTableCopy:
 			srcTable, dstTable := tables[op.U1].References, tables[op.U2].References
diff --git a/internal/wasm/memory.go b/internal/wasm/memory.go
index e2d994f9..bce037e4 100644
--- a/internal/wasm/memory.go
+++ b/internal/wasm/memory.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"math"
 	"reflect"
-	"sync"
 	"unsafe"
 
 	"github.com/tetratelabs/wazero/api"
@@ -37,8 +36,6 @@ type MemoryInstance struct {
 
 	Buffer        []byte
 	Min, Cap, Max uint32
-	// mux is used to prevent overlapping calls to Grow.
-	mux sync.RWMutex
 	// definition is known at compile time.
 	definition api.MemoryDefinition
 }
@@ -180,10 +177,6 @@ func MemoryPagesToBytesNum(pages uint32) (bytesNum uint64) {
 
 // Grow implements the same method as documented on api.Memory.
 func (m *MemoryInstance) Grow(delta uint32) (result uint32, ok bool) {
-	// We take write-lock here as the following might result in a new slice
-	m.mux.Lock()
-	defer m.mux.Unlock()
-
 	currentPages := memoryBytesNumToPages(uint64(len(m.Buffer)))
 	if delta == 0 {
 		return currentPages, true
diff --git a/internal/wasm/store.go b/internal/wasm/store.go
index 4574b491..f15c19fc 100644
--- a/internal/wasm/store.go
+++ b/internal/wasm/store.go
@@ -164,18 +164,17 @@ func (m *ModuleInstance) GetFunctionTypeID(t *FunctionType) FunctionTypeID {
 }
 
 func (m *ModuleInstance) buildElementInstances(elements []ElementSegment) {
-	m.ElementInstances = make([]ElementInstance, len(elements))
+	m.ElementInstances = make([][]Reference, len(elements))
 	for i, elm := range elements {
 		if elm.Type == RefTypeFuncref && elm.Mode == ElementModePassive {
 			// Only passive elements can be access as element instances.
 			// See https://www.w3.org/TR/2022/WD-wasm-core-2-20220419/syntax/modules.html#element-segments
 			inits := elm.Init
-			elemInst := &m.ElementInstances[i]
-			elemInst.References = make([]Reference, len(inits))
-			elemInst.Type = RefTypeFuncref
+			inst := make([]Reference, len(inits))
+			m.ElementInstances[i] = inst
 			for j, idx := range inits {
 				if idx != ElementInitNullReference {
-					elemInst.References[j] = m.Engine.FunctionInstanceReference(idx)
+					inst[j] = m.Engine.FunctionInstanceReference(idx)
 				}
 			}
 		}
diff --git a/internal/wasm/table.go b/internal/wasm/table.go
index b202e8af..ac9fc498 100644
--- a/internal/wasm/table.go
+++ b/internal/wasm/table.go
@@ -3,7 +3,6 @@ package wasm
 import (
 	"fmt"
 	"math"
-	"sync"
 
 	"github.com/tetratelabs/wazero/api"
 	"github.com/tetratelabs/wazero/internal/leb128"
@@ -126,20 +125,12 @@ type TableInstance struct {
 
 	// Type is either RefTypeFuncref or RefTypeExternRef.
 	Type RefType
-
-	// mux is used to prevent overlapping calls to Grow.
-	mux sync.RWMutex
 }
 
 // ElementInstance represents an element instance in a module.
 //
 // See https://www.w3.org/TR/2022/WD-wasm-core-2-20220419/exec/runtime.html#element-instances
-type ElementInstance struct {
-	// References holds references whose type is either RefTypeFuncref or RefTypeExternref (unsupported).
-	References []Reference
-	// Type is the RefType of the references in this instance's References.
-	Type RefType
-}
+type ElementInstance = []Reference
 
 // Reference is the runtime representation of RefType which is either RefTypeFuncref or RefTypeExternref.
 type Reference = uintptr
@@ -314,10 +305,6 @@ func (m *Module) verifyImportGlobalI32(sectionID SectionID, sectionIdx Index, id
 //
 // https://www.w3.org/TR/2022/WD-wasm-core-2-20220419/exec/instructions.html#xref-syntax-instructions-syntax-instr-table-mathsf-table-grow-x
 func (t *TableInstance) Grow(delta uint32, initialRef Reference) (currentLen uint32) {
-	// We take write-lock here as the following might result in a new slice
-	t.mux.Lock()
-	defer t.mux.Unlock()
-
 	currentLen = uint32(len(t.References))
 	if delta == 0 {
 		return


Test patch:
diff --git a/internal/engine/compiler/compiler_post1_0_test.go b/internal/engine/compiler/compiler_post1_0_test.go
index 10a96d3b..7632b354 100644
--- a/internal/engine/compiler/compiler_post1_0_test.go
+++ b/internal/engine/compiler/compiler_post1_0_test.go
@@ -476,13 +476,7 @@ func TestCompiler_compileMemoryInit(t *testing.T) {
 }
 
 func TestCompiler_compileElemDrop(t *testing.T) {
-	origins := []wasm.ElementInstance{
-		{References: []wasm.Reference{1}},
-		{References: []wasm.Reference{2}},
-		{References: []wasm.Reference{3}},
-		{References: []wasm.Reference{4}},
-		{References: []wasm.Reference{5}},
-	}
+	origins := []wasm.ElementInstance{{1}, {2}, {3}, {4}, {5}}
 
 	for i := 0; i < len(origins); i++ {
 		t.Run(strconv.Itoa(i), func(t *testing.T) {
@@ -494,7 +488,7 @@ func TestCompiler_compileElemDrop(t *testing.T) {
 
 			// Verify assumption that before Drop instruction, all the element instances are not empty.
 			for _, inst := range insts {
-				require.NotEqual(t, 0, len(inst.References))
+				require.NotEqual(t, 0, len(inst))
 			}
 
 			compiler := env.requireNewCompiler(t, &wasm.FunctionType{}, newCompiler, &wazeroir.CompilationResult{
@@ -523,9 +517,9 @@ func TestCompiler_compileElemDrop(t *testing.T) {
 
 			for j := 0; j < len(insts); j++ {
 				if i == j {
-					require.Zero(t, len(env.module().ElementInstances[j].References))
+					require.Zero(t, len(env.module().ElementInstances[j]))
 				} else {
-					require.NotEqual(t, 0, len(env.module().ElementInstances[j].References))
+					require.NotEqual(t, 0, len(env.module().ElementInstances[j]))
 				}
 			}
 		})
@@ -627,7 +621,7 @@ func TestCompiler_compileTableCopy(t *testing.T) {
 
 func TestCompiler_compileTableInit(t *testing.T) {
 	elementInstances := []wasm.ElementInstance{
-		{}, {References: []wasm.Reference{1, 2, 3, 4, 5}},
+		{}, {1, 2, 3, 4, 5},
 	}
 
 	const tableSize = 100
@@ -707,8 +701,8 @@ func TestCompiler_compileTableInit(t *testing.T) {
 				for i := 0; i < tableSize; i++ {
 					exp[i] = uintptr(i)
 				}
-				if inst := elementInstances[tc.elemIndex]; inst.References != nil {
-					copy(exp[tc.destOffset:], inst.References[tc.sourceOffset:tc.sourceOffset+tc.copySize])
+				if inst := elementInstances[tc.elemIndex]; inst != nil {
+					copy(exp[tc.destOffset:], inst[tc.sourceOffset:tc.sourceOffset+tc.copySize])
 				}
 				require.Equal(t, exp, table)
 			} else {