Repository: tetratelabs/wazero
Pull request #135 (2022-01-15 06:55:30)

Initialize module instance with `Exports` map
Hi, Thanks for developing a great wasm runtime!

This PR fixes `panic: assignment to entry in nil map` that occurs when `AddGlobal()`, `AddTableInstance()` or `AddMemoryInstance()` is called before `AddHostFunction()`.

This panic does not occur when `AddHostFunction()` called, because `ModuleInstance` is initialized with `Exports` field in `AddHostFunction()` as follows. This PR makes sure that other functions are initialized with `Exports` field as well.
https://github.com/tetratelabs/wazero/blob/6a309797efa5361f45c6a5cb64d567ecf5ceb16d/wasm/store.go#L1637



codefromthecrypt: Thanks for coming by and fixing this @summerwind (song is in my head now!)

Sorry this file doesn't have unit tests, yet. One way to fix both could be to make a function like this and reuse it everywhere. then make a new file store_test.go which has `TestGetModuleInstance` which has cases for when it exists and doesn't. Wanna give it a try?

```go
// getModuleInstance returns an existing ModuleInstance if exists, or assigns a new one.
func (s *Store) getModuleInstance(name string) *ModuleInstance {
	m, ok := s.ModuleInstances[name]
	if !ok {
		m = &ModuleInstance{Exports: map[string]*ExportInstance{}}
		s.ModuleInstances[name] = m
	}
	return m
}
```
summerwind: Thanks for the comment!
That's make sense for me. I'll try to add unit test.
summerwind: @codefromthecrypt Added a unit test as you guided!
Please let me know if there is anything I need to fix.

Patch:
diff --git a/wasm/store.go b/wasm/store.go
index f39aa9eb..7bb20820 100644
--- a/wasm/store.go
+++ b/wasm/store.go
@@ -334,7 +334,7 @@ func (s *Store) executeConstExpression(target *ModuleInstance, expr *ConstantExp
 	case OpcodeF32Const:
 		v, err = readFloat32(r)
 		if err != nil {
-			return nil, 0, fmt.Errorf("read f34: %w", err)
+			return nil, 0, fmt.Errorf("read f32: %w", err)
 		}
 		return v, ValueTypeF32, nil
 	case OpcodeF64Const:
@@ -1632,13 +1632,9 @@ func (s *Store) AddHostFunction(moduleName, funcName string, fn reflect.Value) e
 		return &FunctionType{Params: paramTypes, Results: resultTypes}, nil
 	}
 
-	m, ok := s.ModuleInstances[moduleName]
-	if !ok {
-		m = &ModuleInstance{Exports: map[string]*ExportInstance{}}
-		s.ModuleInstances[moduleName] = m
-	}
+	m := s.getModuleInstance(moduleName)
 
-	_, ok = m.Exports[funcName]
+	_, ok := m.Exports[funcName]
 	if ok {
 		return fmt.Errorf("name %s already exists in module %s", funcName, moduleName)
 	}
@@ -1667,13 +1663,9 @@ func (s *Store) AddHostFunction(moduleName, funcName string, fn reflect.Value) e
 }
 
 func (s *Store) AddGlobal(moduleName, name string, value uint64, valueType ValueType, mutable bool) error {
-	m, ok := s.ModuleInstances[moduleName]
-	if !ok {
-		m = &ModuleInstance{}
-		s.ModuleInstances[moduleName] = m
-	}
+	m := s.getModuleInstance(moduleName)
 
-	_, ok = m.Exports[name]
+	_, ok := m.Exports[name]
 	if ok {
 		return fmt.Errorf("name %s already exists in module %s", name, moduleName)
 	}
@@ -1687,13 +1679,9 @@ func (s *Store) AddGlobal(moduleName, name string, value uint64, valueType Value
 }
 
 func (s *Store) AddTableInstance(moduleName, name string, min uint32, max *uint32) error {
-	m, ok := s.ModuleInstances[moduleName]
-	if !ok {
-		m = &ModuleInstance{}
-		s.ModuleInstances[moduleName] = m
-	}
+	m := s.getModuleInstance(moduleName)
 
-	_, ok = m.Exports[name]
+	_, ok := m.Exports[name]
 	if ok {
 		return fmt.Errorf("name %s already exists in module %s", name, moduleName)
 	}
@@ -1710,13 +1698,9 @@ func (s *Store) AddTableInstance(moduleName, name string, min uint32, max *uint3
 }
 
 func (s *Store) AddMemoryInstance(moduleName, name string, min uint32, max *uint32) error {
-	m, ok := s.ModuleInstances[moduleName]
-	if !ok {
-		m = &ModuleInstance{}
-		s.ModuleInstances[moduleName] = m
-	}
+	m := s.getModuleInstance(moduleName)
 
-	_, ok = m.Exports[name]
+	_, ok := m.Exports[name]
 	if ok {
 		return fmt.Errorf("name %s already exists in module %s", name, moduleName)
 	}
@@ -1730,3 +1714,13 @@ func (s *Store) AddMemoryInstance(moduleName, name string, min uint32, max *uint
 	s.Memories = append(s.Memories, memory)
 	return nil
 }
+
+// getModuleInstance returns an existing ModuleInstance if exists, or assigns a new one.
+func (s *Store) getModuleInstance(name string) *ModuleInstance {
+	m, ok := s.ModuleInstances[name]
+	if !ok {
+		m = &ModuleInstance{Exports: map[string]*ExportInstance{}}
+		s.ModuleInstances[name] = m
+	}
+	return m
+}


Test patch:
diff --git a/wasm/store_test.go b/wasm/store_test.go
new file mode 100644
index 00000000..659e4ca8
--- /dev/null
+++ b/wasm/store_test.go
@@ -0,0 +1,22 @@
+package wasm
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGetModuleInstance(t *testing.T) {
+	name := "test"
+
+	// 'jit' and 'wazeroir' package cannot be used because of circular import.
+	// Here we will use 'nil' instead. Should we have an Engine for testing?
+	s := NewStore(nil)
+
+	m1 := s.getModuleInstance(name)
+	require.Equal(t, m1, s.ModuleInstances[name])
+	require.NotNil(t, m1.Exports)
+
+	m2 := s.getModuleInstance(name)
+	require.Equal(t, m1, m2)
+}