Repository: tetratelabs/wazero
Pull request #374 (2022-03-15 03:09:28)

Propagates module name to module engine panic nicer
Before, if there was an unexpected panic on munmap of a code section,
we'd only see "invalid argument" and have to guess why. This propagates
context and stubs out the finalizer so we can test a better error, like:

"jit: failed to munmap code segment for test.function[0]: invalid argument"

This also attempts to improve troubleshooting time by making all module
instantiation that used empty module name, use the test name instead.


codefromthecrypt: > just curious: is it a common practice in Golang for a non-IO interface to adopt io.Closer because it has Close() error?

good question. What I'm hoping for really is static analysis that helps you detect something you forgot to close. That said, haven't found a linter yet (ex not in golint-ci) https://github.com/dominikh/go-tools/issues/139

I'll add a comment why
codefromthecrypt: I added code to panic on zero length code segments because that's a bug on our end (even in tests) as there's no valid code length zero (opcodeend is required). I also backfilled tests on freeing memory never mapped.

Patch:
diff --git a/internal/wasm/engine.go b/internal/wasm/engine.go
index a5dc584d..eb352dec 100644
--- a/internal/wasm/engine.go
+++ b/internal/wasm/engine.go
@@ -1,20 +1,27 @@
 package internalwasm
 
+import "io"
+
 // Engine is a Store-scoped mechanism to compile functions declared or imported by a module.
 // This is a top-level type implemented by an interpreter or JIT compiler.
 type Engine interface {
 	// NewModuleEngine compiles down the function instances in a module, and returns ModuleEngine for the module.
 	//
+	// * name is the name the module was instantiated with used for error handling.
 	// * importedFunctions: functions this module imports, already compiled in this engine.
 	// * moduleFunctions: functions declared in this module that must be compiled.
 	//
 	// Note: Input parameters must be pre-validated with internalwasm.Module Validate, to ensure no fields are invalid
 	// due to reasons such as out-of-bounds.
-	NewModuleEngine(importedFunctions, moduleFunctions []*FunctionInstance) (ModuleEngine, error)
+	NewModuleEngine(name string, importedFunctions, moduleFunctions []*FunctionInstance) (ModuleEngine, error)
 }
 
 // ModuleEngine implements function calls for a given module.
 type ModuleEngine interface {
+	// Closer releases the resources allocated by functions in this ModuleEngine.
+	io.Closer
+	// ^^ io.Closer not due to I/O, but to allow future static analysis to catch leaks (unclosed Closers).
+
 	// FunctionAddress returns the absolute address of the compiled function for index.
 	// The returned address is stored and used as a TableInstance.Table element.
 	//
@@ -25,7 +32,4 @@ type ModuleEngine interface {
 	// Returns the results from the function.
 	// The ctx's context.Context will be the outer-most ancestor of the argument to wasm.Function.
 	Call(ctx *ModuleContext, f *FunctionInstance, params ...uint64) (results []uint64, err error)
-
-	// Close releases the resources allocated by functions in this ModuleEngine.
-	Close()
 }
diff --git a/internal/wasm/interpreter/interpreter.go b/internal/wasm/interpreter/interpreter.go
index 2ffcbcde..d378e51d 100644
--- a/internal/wasm/interpreter/interpreter.go
+++ b/internal/wasm/interpreter/interpreter.go
@@ -52,6 +52,9 @@ func (e *engine) addCompiledFunction(f *wasm.FunctionInstance, cf *compiledFunct
 
 // moduleEngine implements internalwasm.ModuleEngine
 type moduleEngine struct {
+	// name is the name the module was instantiated with used for error handling.
+	name string
+
 	// compiledFunctions are the compiled functions in a module instances.
 	// The index is module instance-scoped.
 	compiledFunctions []*compiledFunction
@@ -148,17 +151,19 @@ type interpreterOp struct {
 	rs     []*wazeroir.InclusiveRange
 }
 
-// Close implements wasm.Engine Close
-func (me *moduleEngine) Close() {
+// Close implements io.Closer
+func (me *moduleEngine) Close() (err error) {
 	// Release all the function instances declared in this module.
 	for _, cf := range me.compiledFunctions[me.importedFunctionCounts:] {
 		me.parentEngine.deleteCompiledFunction(cf.funcInstance)
 	}
+	return
 }
 
 // NewModuleEngine implements internalwasm.Engine NewModuleEngine
-func (e *engine) NewModuleEngine(importedFunctions, moduleFunctions []*wasm.FunctionInstance) (wasm.ModuleEngine, error) {
+func (e *engine) NewModuleEngine(name string, importedFunctions, moduleFunctions []*wasm.FunctionInstance) (wasm.ModuleEngine, error) {
 	me := &moduleEngine{
+		name:                   name,
 		compiledFunctions:      make([]*compiledFunction, 0, len(importedFunctions)+len(moduleFunctions)),
 		parentEngine:           e,
 		importedFunctionCounts: len(importedFunctions),
diff --git a/internal/wasm/jit/engine.go b/internal/wasm/jit/engine.go
index 1fdc2e3f..12c20d71 100644
--- a/internal/wasm/jit/engine.go
+++ b/internal/wasm/jit/engine.go
@@ -25,6 +25,9 @@ type (
 
 	// moduleEngine implements internalwasm.ModuleEngine
 	moduleEngine struct {
+		// name is the name the module was instantiated with used for error handling.
+		name string
+
 		// compiledFunctions are the compiled functions in a module instances.
 		// The index is module instance-scoped. We intentionally avoid using map
 		// as the underlying memory region is accessed by assembly directly by using
@@ -194,7 +197,7 @@ type (
 // See TestVerifyOffsetValue for how to derive these values.
 const (
 	// Offsets for moduleEngine.compiledFunctions
-	moduleEngineCompiledFunctionsOffset = 0
+	moduleEngineCompiledFunctionsOffset = 16
 
 	// Offsets for callEngine globalContext.
 	callEngineGlobalContextValueStackElement0AddressOffset     = 0
@@ -329,9 +332,10 @@ func (c *callFrame) String() string {
 }
 
 // NewModuleEngine implements internalwasm.Engine NewModuleEngine
-func (e *engine) NewModuleEngine(importedFunctions, moduleFunctions []*wasm.FunctionInstance) (wasm.ModuleEngine, error) {
+func (e *engine) NewModuleEngine(name string, importedFunctions, moduleFunctions []*wasm.FunctionInstance) (wasm.ModuleEngine, error) {
 	imported := len(importedFunctions)
 	me := &moduleEngine{
+		name:                   name,
 		compiledFunctions:      make([]*compiledFunction, 0, imported+len(moduleFunctions)),
 		parentEngine:           e,
 		importedFunctionCounts: imported,
@@ -372,37 +376,49 @@ func (me *moduleEngine) FunctionAddress(index wasm.Index) uintptr {
 	return uintptr(unsafe.Pointer(me.compiledFunctions[index]))
 }
 
-// Close implements internalwasm.ModuleEngine Close
-func (me *moduleEngine) Close() {
-	// Current impl: Close is currently guarded with CAS to only be called once. There is a race-condition inside the
-	// critical section: functions are removed from the parent engine, but there's no guard to prevent this
-	// moduleInstance from making new calls. This means at inside the critical section there could be in-flight calls,
-	// and even after it new calls can be made, given a reference to this moduleEngine.
-	//
-	// To ensure neither in-flight, nor new calls segfault due to missing code segment, memory isn't unmapped here.
-	// Rather, it is added to the finalizer queue to be done at some point (perhaps never). This needs to eventually be
-	// as close leaks resources until cleaned up by the finalizer which by docs may never be run.
-	//
-	// Potential future design (possibly faulty, so expect impl to be more complete or better):
-	//  * Change this to implement io.Closer and document this is blocking
-	//    * This implies adding docs can suggest this is run in a goroutine
-	//    * io.Closer allows an error return we can use in case an unrecoverable error happens
-	//  * Continue to guard with CAS so that close is only executed once
-	//  * Once in the critical section, write a status bit to a fixed memory location.
-	//    * End new calls with a Closed Error if this is read.
-	//    * This guard allows Close to eventually complete.
-	//  * Block exiting the critical section until all in-flight calls complete.
-	//    * Knowing which in-flight calls from other modules, that can use this module may be tricky
-	//    * Pure wasm functions can be left to complete.
-	//    * Host functions are the only unknowns (ex can do I/O) so they may need to be tracked.
+// Close implements io.Closer
+func (me *moduleEngine) Close() (err error) {
+	me.doClose(runtime.SetFinalizer) // extracted for testability
+	return
+}
 
+// doClose is currently guarded with CAS to only be called once. There is a race-condition inside the
+// critical section: functions are removed from the parent engine, but there's no guard to prevent this
+// moduleInstance from making new calls. This means at inside the critical section there could be in-flight calls,
+// and even after it new calls can be made, given a reference to this moduleEngine.
+//
+// To ensure neither in-flight, nor new calls segfault due to missing code segment, memory isn't unmapped here.
+// Rather, it is added to the finalizer queue to be done at some point (perhaps never). This needs to eventually be
+// as close leaks resources until cleaned up by the finalizer which by docs may never be run.
+//
+// Potential future design (possibly faulty, so expect impl to be more complete or better):
+//  * Change this to implement io.Closer and document this is blocking
+//    * This implies adding docs can suggest this is run in a goroutine
+//    * io.Closer allows an error return we can use in case an unrecoverable error happens
+//  * Continue to guard with CAS so that close is only executed once
+//  * Once in the critical section, write a status bit to a fixed memory location.
+//    * End new calls with a Closed Error if this is read.
+//    * This guard allows Close to eventually complete.
+//  * Block exiting the critical section until all in-flight calls complete.
+//    * Knowing which in-flight calls from other modules, that can use this module may be tricky
+//    * Pure wasm functions can be left to complete.
+//    * Host functions are the only unknowns (ex can do I/O) so they may need to be tracked.
+func (me *moduleEngine) doClose(setFinalizer func(obj interface{}, finalizer interface{})) {
 	// Setting finalizer multiple times result in panic, so we guard with CAS.
 	if atomic.CompareAndSwapUint32(&me.closed, 0, 1) {
+		moduleName := me.name // pin to avoid leaking moduleEngine on any finalizer error.
+
 		// Release all the function instances declared in this module.
-		for _, cf := range me.compiledFunctions[me.importedFunctionCounts:] {
-			runtime.SetFinalizer(cf, func(compiledFn *compiledFunction) {
+		for i, cf := range me.compiledFunctions[me.importedFunctionCounts:] {
+			funcIdx := wasm.Index(i) // pin for any finalizer error
+
+			setFinalizer(cf, func(compiledFn *compiledFunction) {
 				if err := munmapCodeSegment(compiledFn.codeSegment); err != nil {
-					panic(err) // munmap failure cannot recover.
+
+					// munmap failure cannot recover, and it happens asynchronously. Add context for troubleshooting.
+					// module+funcidx should be enough, but if not, we can later propagate the func name.
+					err = fmt.Errorf("jit: failed to munmap code segment for %s.function[%d]: %w", moduleName, funcIdx, err)
+					panic(err)
 				}
 			})
 			me.parentEngine.deleteCompiledFunction(cf.source)
@@ -506,21 +522,21 @@ func newEngine() *engine {
 // dangerous memory access from native code.
 //
 // Background: Go has a mechanism called "goroutine stack-shrink" where Go
-// runtime shrinks Gorotuine's stack when it is GCing. Shrinking means that
+// runtime shrinks Goroutine's stack when it is GCing. Shrinking means that
 // all the contents on the goroutine stack will be relocated by runtime,
 // Therefore, the memory address of these contents change undeterministically.
-// Not only shrinks, but also Goruntime grows the goroutine stack at any point
+// Not only shrinks, but also Go runtime grows the goroutine stack at any point
 // of function call entries, which also might end up relocating contents.
 //
 // On the other hand, we hold pointers to the data region of value stack and
-// callframe stack slices and use these raw pointers from native code.
+// call-frame stack slices and use these raw pointers from native code.
 // Therefore, it is dangerous if these two stacks are allocated on stack
 // as these stack's address might be changed by Goruntime which we cannot
 // detect.
 //
-// By declaring theses values as `var`, slices created via `make([]..., var)`
+// By declaring these values as `var`, slices created via `make([]..., var)`
 // will never be allocated on stack [1]. This means accessing these slices via
-// raw pointers is safe: As of version 1.18, Go's garbage collectornever relocates
+// raw pointers is safe: As of version 1.18, Go's garbage collector never relocates
 // heap-allocated objects (aka no compilation of memory [2]).
 //
 // On Go upgrades, re-validate heap-allocation via `go build -gcflags='-m' ./internal/wasm/jit/...`.
diff --git a/internal/wasm/jit/mmap.go b/internal/wasm/jit/mmap.go
index 8ea2b321..4236e72a 100644
--- a/internal/wasm/jit/mmap.go
+++ b/internal/wasm/jit/mmap.go
@@ -3,6 +3,7 @@
 package jit
 
 import (
+	"errors"
 	"runtime"
 	"syscall"
 )
@@ -10,6 +11,9 @@ import (
 // mmapCodeSegment copies the code into the executable region and returns the byte slice of the region.
 // See https://man7.org/linux/man-pages/man2/mmap.2.html for mmap API and flags.
 func mmapCodeSegment(code []byte) ([]byte, error) {
+	if len(code) == 0 {
+		panic(errors.New("BUG: mmapCodeSegment with zero length"))
+	}
 	if runtime.GOARCH == "amd64" {
 		return mmapCodeSegmentAMD64(code)
 	} else {
@@ -19,6 +23,9 @@ func mmapCodeSegment(code []byte) ([]byte, error) {
 
 // munmapCodeSegment unmaps the given memory region.
 func munmapCodeSegment(code []byte) error {
+	if len(code) == 0 {
+		panic(errors.New("BUG: munmapCodeSegment with zero length"))
+	}
 	return syscall.Munmap(code)
 }
 
diff --git a/internal/wasm/jit/mmap_windows.go b/internal/wasm/jit/mmap_windows.go
index 9bdde14a..a7d60879 100644
--- a/internal/wasm/jit/mmap_windows.go
+++ b/internal/wasm/jit/mmap_windows.go
@@ -1,6 +1,8 @@
 package jit
 
 import (
+	"errors"
+	"fmt"
 	"reflect"
 	"runtime"
 	"syscall"
@@ -12,17 +14,21 @@ var (
 	procVirtualAlloc   = kernel32.NewProc("VirtualAlloc")
 	procVirtualProtect = kernel32.NewProc("VirtualProtect")
 	procVirtualFree    = kernel32.NewProc("VirtualFree")
+	procGetLastError   = kernel32.NewProc("GetLastError")
 )
 
 const (
-	windows_MEM_COMMIT             = 0x00001000
-	windows_MEM_RELEASE            = 0x00008000
-	windows_PAGE_READWRITE         = 0x00000004
-	windows_PAGE_EXECUTE_READ      = 0x00000020
-	windows_PAGE_EXECUTE_READWRITE = 0x00000040
+	windows_MEM_COMMIT             uintptr = 0x00001000
+	windows_MEM_RELEASE            uintptr = 0x00008000
+	windows_PAGE_READWRITE         uintptr = 0x00000004
+	windows_PAGE_EXECUTE_READ      uintptr = 0x00000020
+	windows_PAGE_EXECUTE_READWRITE uintptr = 0x00000040
 )
 
 func mmapCodeSegment(code []byte) ([]byte, error) {
+	if len(code) == 0 {
+		panic(errors.New("BUG: mmapCodeSegment with zero length"))
+	}
 	if runtime.GOARCH == "amd64" {
 		return mmapCodeSegmentAMD64(code)
 	} else {
@@ -31,39 +37,46 @@ func mmapCodeSegment(code []byte) ([]byte, error) {
 }
 
 func munmapCodeSegment(code []byte) error {
-	// size must be 0 because we're using MEM_RELEASE.
-	// See https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualfree
-	return virtualFree(uintptr(unsafe.Pointer(&code[0])), 0, windows_MEM_RELEASE)
+	if len(code) == 0 {
+		panic(errors.New("BUG: munmapCodeSegment with zero length"))
+	}
+	return freeMemory(code)
 }
 
-func virtualAlloc(address uintptr, size uintptr, alloctype uint32, protect uint32) (uintptr, error) {
-	r0, _, err := procVirtualAlloc.Call(address, size, uintptr(alloctype), uintptr(protect))
-	if r0 == 0 {
-		return 0, err
+// allocateMemory commits the memory region via the "VirtualAlloc" function.
+// See https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualalloc
+func allocateMemory(code []byte, protect uintptr) (uintptr, error) {
+	address := uintptr(0) // TODO: document why zero
+	size := uintptr(len(code))
+	alloctype := windows_MEM_COMMIT
+	if r, _, _ := procVirtualAlloc.Call(address, size, alloctype, protect); r == 0 {
+		return 0, fmt.Errorf("jit: VirtualAlloc error: %w", getLastError())
+	} else {
+		return r, nil
 	}
-	return r0, nil
 }
 
-func virtualProtect(address uintptr, size uintptr, newprotect uint32, oldprotect *uint32) error {
-	r1, _, e1 := procVirtualProtect.Call(address, size, uintptr(newprotect), uintptr(unsafe.Pointer(oldprotect)))
-	if r1 == 0 {
-		return e1
+// freeMemory releases the memory region via the "VirtualFree" function.
+// See https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualfree
+func freeMemory(code []byte) error {
+	address := uintptr(unsafe.Pointer(&code[0]))
+	size := uintptr(0) // size must be 0 because we're using MEM_RELEASE.
+	freetype := windows_MEM_RELEASE
+	if r, _, _ := procVirtualFree.Call(address, size, freetype); r == 0 {
+		return fmt.Errorf("jit: VirtualFree error: %w", getLastError())
 	}
 	return nil
 }
 
-func virtualFree(address uintptr, size uintptr, freetype uint32) error {
-	r1, _, e1 := procVirtualFree.Call(address, size, uintptr(freetype))
-	// 0 indicates failure.
-	// See https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualfree#return-value
-	if r1 == 0 {
-		return e1
+func virtualProtect(address, size, newprotect uintptr, oldprotect *uint32) error {
+	if r, _, _ := procVirtualProtect.Call(address, size, newprotect, uintptr(unsafe.Pointer(oldprotect))); r == 0 {
+		return fmt.Errorf("jit: VirtualProtect error: %w", getLastError())
 	}
 	return nil
 }
 
 func mmapCodeSegmentAMD64(code []byte) ([]byte, error) {
-	p, err := virtualAlloc(0, uintptr(len(code)), windows_MEM_COMMIT, windows_PAGE_EXECUTE_READWRITE)
+	p, err := allocateMemory(code, windows_PAGE_EXECUTE_READWRITE)
 	if err != nil {
 		return nil, err
 	}
@@ -78,7 +91,7 @@ func mmapCodeSegmentAMD64(code []byte) ([]byte, error) {
 }
 
 func mmapCodeSegmentARM64(code []byte) ([]byte, error) {
-	p, err := virtualAlloc(0, uintptr(len(code)), windows_MEM_COMMIT, windows_PAGE_READWRITE)
+	p, err := allocateMemory(code, windows_PAGE_READWRITE)
 	if err != nil {
 		return nil, err
 	}
@@ -97,3 +110,14 @@ func mmapCodeSegmentARM64(code []byte) ([]byte, error) {
 	}
 	return mem, nil
 }
+
+// getLastError casts the last error on the calling thread to a syscall.Errno or returns syscall.EINVAL.
+//
+// See https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror
+// See https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes
+func getLastError() syscall.Errno {
+	if errno, _, _ := procGetLastError.Call(); errno != 0 {
+		return syscall.Errno(errno)
+	}
+	return syscall.EINVAL
+}
diff --git a/internal/wasm/store.go b/internal/wasm/store.go
index 558281be..af859d06 100644
--- a/internal/wasm/store.go
+++ b/internal/wasm/store.go
@@ -299,7 +299,7 @@ func (s *Store) Instantiate(ctx context.Context, module *Module, name string) (*
 		globals, importedTable, table, importedMemory, memory, types)
 
 	// Plus we are ready to compile functions.
-	m.Engine, err = s.engine.NewModuleEngine(importedFunctions, functions)
+	m.Engine, err = s.engine.NewModuleEngine(name, importedFunctions, functions)
 	if err != nil {
 		s.deleteModule(name)
 		return nil, fmt.Errorf("compilation failed: %w", err)


Test patch:
diff --git a/internal/wasm/global_test.go b/internal/wasm/global_test.go
index 35b35c7e..215f2bc6 100644
--- a/internal/wasm/global_test.go
+++ b/internal/wasm/global_test.go
@@ -260,7 +260,7 @@ func TestPublicModule_Global(t *testing.T) {
 		s := newStore()
 		t.Run(tc.name, func(t *testing.T) {
 			// Instantiate the module and get the export of the above global
-			module, err := s.Instantiate(context.Background(), tc.module, "")
+			module, err := s.Instantiate(context.Background(), tc.module, t.Name())
 			require.NoError(t, err)
 
 			if global := module.ExportedGlobal("global"); tc.expected != nil {
diff --git a/internal/wasm/interpreter/interpreter_test.go b/internal/wasm/interpreter/interpreter_test.go
index b598170f..47ed177b 100644
--- a/internal/wasm/interpreter/interpreter_test.go
+++ b/internal/wasm/interpreter/interpreter_test.go
@@ -47,6 +47,17 @@ func TestCallEngine_PushFrame_StackOverflow(t *testing.T) {
 	require.Panics(t, func() { vm.pushFrame(f4) })
 }
 
+func TestEngine_NewModuleEngine(t *testing.T) {
+	e := NewEngine()
+
+	t.Run("sets module name", func(t *testing.T) {
+		me, err := e.NewModuleEngine(t.Name(), nil, nil)
+		require.NoError(t, err)
+		defer me.Close()
+		require.Equal(t, t.Name(), me.(*moduleEngine).name)
+	})
+}
+
 func TestEngine_Call(t *testing.T) {
 	i64 := wasm.ValueTypeI64
 	m := &wasm.Module{
@@ -59,7 +70,7 @@ func TestEngine_Call(t *testing.T) {
 	// Use exported functions to simplify instantiation of a Wasm function
 	e := NewEngine()
 	store := wasm.NewStore(e, wasm.Features20191205)
-	mod, err := store.Instantiate(context.Background(), m, "")
+	mod, err := store.Instantiate(context.Background(), m, t.Name())
 	require.NoError(t, err)
 
 	fn := mod.ExportedFunction("fn")
@@ -102,7 +113,7 @@ func TestEngine_Call_HostFn(t *testing.T) {
 		Module: module,
 	}
 
-	me, err := e.NewModuleEngine(nil, []*wasm.FunctionInstance{f})
+	me, err := e.NewModuleEngine(t.Name(), nil, []*wasm.FunctionInstance{f})
 	require.NoError(t, err)
 
 	t.Run("defaults to module memory when call stack empty", func(t *testing.T) {
@@ -239,7 +250,7 @@ func TestCallEngine_callNativeFunc_signExtend(t *testing.T) {
 func TestEngineCompile_Errors(t *testing.T) {
 	t.Run("invalid import", func(t *testing.T) {
 		e := NewEngine().(*engine)
-		_, err := e.NewModuleEngine(
+		_, err := e.NewModuleEngine(t.Name(),
 			[]*wasm.FunctionInstance{{Module: &wasm.ModuleInstance{Name: "uncompiled"}, Name: "fn"}},
 			nil, // moduleFunctions
 		)
@@ -257,7 +268,7 @@ func TestEngineCompile_Errors(t *testing.T) {
 		}
 
 		// initialize the module-engine containing imported functions
-		_, err := e.NewModuleEngine(nil, importedFunctions)
+		_, err := e.NewModuleEngine(t.Name(), nil, importedFunctions)
 		require.NoError(t, err)
 
 		require.Len(t, e.compiledFunctions, len(importedFunctions))
@@ -270,7 +281,7 @@ func TestEngineCompile_Errors(t *testing.T) {
 			}, Module: &wasm.ModuleInstance{}},
 		}
 
-		_, err = e.NewModuleEngine(importedFunctions, moduleFunctions)
+		_, err = e.NewModuleEngine(t.Name(), importedFunctions, moduleFunctions)
 		require.EqualError(t, err, "function[2/2] failed to lower to wazeroir: handling instruction: apply stack failed for call: reading immediates: EOF")
 
 		// On the compilation failure, all the compiled functions including succeeded ones must be released.
@@ -310,12 +321,12 @@ func TestClose(t *testing.T) {
 			e := NewEngine().(*engine)
 			if len(tc.importedFunctions) > 0 {
 				// initialize the module-engine containing imported functions
-				me, err := e.NewModuleEngine(nil, tc.importedFunctions)
+				me, err := e.NewModuleEngine(t.Name(), nil, tc.importedFunctions)
 				require.NoError(t, err)
 				require.Len(t, me.(*moduleEngine).compiledFunctions, len(tc.importedFunctions))
 			}
 
-			me, err := e.NewModuleEngine(tc.importedFunctions, tc.moduleFunctions)
+			me, err := e.NewModuleEngine(t.Name(), tc.importedFunctions, tc.moduleFunctions)
 			require.NoError(t, err)
 			require.Len(t, me.(*moduleEngine).compiledFunctions, len(tc.importedFunctions)+len(tc.moduleFunctions))
 
diff --git a/internal/wasm/jit/engine_test.go b/internal/wasm/jit/engine_test.go
index f76c9667..fcd1b1c7 100644
--- a/internal/wasm/jit/engine_test.go
+++ b/internal/wasm/jit/engine_test.go
@@ -2,6 +2,7 @@ package jit
 
 import (
 	"context"
+	"fmt"
 	"math"
 	"reflect"
 	"runtime"
@@ -99,6 +100,19 @@ func TestVerifyOffsetValue(t *testing.T) {
 	require.Equal(t, int(unsafe.Offsetof(eface.data)), interfaceDataOffset)
 }
 
+func TestEngine_NewModuleEngine(t *testing.T) {
+	requireSupportedOSArch(t)
+
+	e := NewEngine()
+
+	t.Run("sets module name", func(t *testing.T) {
+		me, err := e.NewModuleEngine(t.Name(), nil, nil)
+		require.NoError(t, err)
+		defer me.Close()
+		require.Equal(t, t.Name(), me.(*moduleEngine).name)
+	})
+}
+
 func TestEngine_Call(t *testing.T) {
 	requireSupportedOSArch(t)
 
@@ -113,7 +127,7 @@ func TestEngine_Call(t *testing.T) {
 	// Use exported functions to simplify instantiation of a Wasm function
 	e := NewEngine()
 	store := wasm.NewStore(e, wasm.Features20191205)
-	mod, err := store.Instantiate(context.Background(), m, "")
+	mod, err := store.Instantiate(context.Background(), m, t.Name())
 	require.NoError(t, err)
 
 	fn := mod.ExportedFunction("fn")
@@ -158,7 +172,7 @@ func TestEngine_Call_HostFn(t *testing.T) {
 		Module: module,
 	}
 
-	modEngine, err := e.NewModuleEngine(nil, []*wasm.FunctionInstance{f})
+	modEngine, err := e.NewModuleEngine(t.Name(), nil, []*wasm.FunctionInstance{f})
 	require.NoError(t, err)
 
 	t.Run("defaults to module memory when call stack empty", func(t *testing.T) {
@@ -189,7 +203,11 @@ func requireSupportedOSArch(t *testing.T) {
 func TestEngineCompile_Errors(t *testing.T) {
 	t.Run("invalid import", func(t *testing.T) {
 		e := newEngine()
-		_, err := e.NewModuleEngine([]*wasm.FunctionInstance{{Module: &wasm.ModuleInstance{Name: "uncompiled"}, Name: "fn"}}, nil)
+		_, err := e.NewModuleEngine(
+			t.Name(),
+			[]*wasm.FunctionInstance{{Module: &wasm.ModuleInstance{Name: "uncompiled"}, Name: "fn"}},
+			nil, // moduleFunctions
+		)
 		require.EqualError(t, err, "import[0] func[uncompiled.fn]: uncompiled")
 	})
 
@@ -202,7 +220,7 @@ func TestEngineCompile_Errors(t *testing.T) {
 			{Name: "3", Type: &wasm.FunctionType{}, Body: []byte{wasm.OpcodeEnd}, Module: &wasm.ModuleInstance{}},
 			{Name: "4", Type: &wasm.FunctionType{}, Body: []byte{wasm.OpcodeEnd}, Module: &wasm.ModuleInstance{}},
 		}
-		_, err := e.NewModuleEngine(nil, importedFunctions)
+		_, err := e.NewModuleEngine(t.Name(), nil, importedFunctions)
 		require.NoError(t, err)
 
 		require.Len(t, e.compiledFunctions, len(importedFunctions))
@@ -215,7 +233,7 @@ func TestEngineCompile_Errors(t *testing.T) {
 			}, Module: &wasm.ModuleInstance{}},
 		}
 
-		_, err = e.NewModuleEngine(importedFunctions, moduleFunctions)
+		_, err = e.NewModuleEngine(t.Name(), importedFunctions, moduleFunctions)
 		require.EqualError(t, err, "function[2/2] failed to lower to wazeroir: handling instruction: apply stack failed for call: reading immediates: EOF")
 
 		// On the compilation failure, all the compiled functions including succeeded ones must be released.
@@ -226,7 +244,44 @@ func TestEngineCompile_Errors(t *testing.T) {
 	})
 }
 
-func TestRelease(t *testing.T) {
+var fakeFinalizer = func(obj interface{}, finalizer interface{}) {
+	cf := obj.(*compiledFunction)
+	fn := finalizer.(func(compiledFn *compiledFunction))
+	fn(cf)
+}
+
+// TestModuleEngine_Close_Panic tests that an unexpected panic has some identifying information in it.
+func TestModuleEngine_Close_Panic(t *testing.T) {
+	e := newEngine()
+	me := &moduleEngine{
+		name: t.Name(),
+		compiledFunctions: []*compiledFunction{
+			{codeSegment: []byte{wasm.OpcodeEnd} /* invalid because not compiled */},
+		},
+		parentEngine: e,
+	}
+
+	// capturePanic because there's no require.PanicsWithErrorPrefix
+	errMessage := capturePanic(func() {
+		me.doClose(fakeFinalizer)
+	})
+	require.Contains(t, errMessage.Error(), "jit: failed to munmap code segment for TestModuleEngine_Close_Panic.function[0]:")
+}
+
+// capturePanic returns an error recovered from a panic
+func capturePanic(panics func()) (err error) {
+	defer func() {
+		if recovered := recover(); recovered != nil {
+			if e, ok := recovered.(error); ok {
+				err = e
+			}
+		}
+	}()
+	panics()
+	return
+}
+
+func TestModuleEngine_Close_Concurrent(t *testing.T) {
 	newFunctionInstance := func(id int) *wasm.FunctionInstance {
 		return &wasm.FunctionInstance{
 			Name: strconv.Itoa(id), Type: &wasm.FunctionType{}, Body: []byte{wasm.OpcodeEnd}, Module: &wasm.ModuleInstance{}}
@@ -255,13 +310,13 @@ func TestRelease(t *testing.T) {
 			e := newEngine()
 			var importedModuleEngine *moduleEngine
 			if len(tc.importedFunctions) > 0 {
-				modEngine, err := e.NewModuleEngine(nil, tc.importedFunctions)
+				modEngine, err := e.NewModuleEngine(fmt.Sprintf("%s - imported functions", t.Name()), nil, tc.importedFunctions)
 				require.NoError(t, err)
 				importedModuleEngine = modEngine.(*moduleEngine)
 				require.Len(t, importedModuleEngine.compiledFunctions, len(tc.importedFunctions))
 			}
 
-			modEngine, err := e.NewModuleEngine(tc.importedFunctions, tc.moduleFunctions)
+			modEngine, err := e.NewModuleEngine(fmt.Sprintf("%s - module-defined functions", t.Name()), tc.importedFunctions, tc.moduleFunctions)
 			require.NoError(t, err)
 			require.Len(t, modEngine.(*moduleEngine).compiledFunctions, len(tc.importedFunctions)+len(tc.moduleFunctions))
 
@@ -284,7 +339,9 @@ func TestRelease(t *testing.T) {
 			for i := 0; i < goroutines; i++ {
 				go func() {
 					defer wg.Done()
-					modEngine.Close() // concurrent mutliple execution of Close must be guarded by atomic.
+
+					// Ensure concurrent multiple execution of Close is guarded by atomic without overloading finalizer.
+					modEngine.(*moduleEngine).doClose(fakeFinalizer)
 				}()
 			}
 			wg.Wait()
@@ -298,11 +355,6 @@ func TestRelease(t *testing.T) {
 				require.NotContains(t, e.compiledFunctions, f, i)
 			}
 
-			// Release the reference to ModuleEngine -> *compiledFunctions for non-imported compiledFunctions
-			// must be munmapped.
-			modEngine = nil
-			runtime.GC()
-
 			for _, mappedRegion := range mappedRegions {
 				// munmap twice should result in error.
 				err = munmapCodeSegment(mappedRegion)
@@ -310,9 +362,7 @@ func TestRelease(t *testing.T) {
 			}
 
 			if len(tc.importedFunctions) > 0 {
-				importedModuleEngine.Close()
-				importedModuleEngine = nil
-				runtime.GC()
+				importedModuleEngine.doClose(fakeFinalizer)
 
 				for _, mappedRegion := range importedMappedRegions {
 					// munmap twice should result in error.
@@ -403,7 +453,7 @@ func TestSliceAllocatedOnHeap(t *testing.T) {
 		},
 	}
 
-	mi, err := store.Instantiate(context.Background(), m, "")
+	mi, err := store.Instantiate(context.Background(), m, t.Name())
 	require.NoError(t, err)
 
 	for _, fnName := range []string{valueStackCorruption, callStackCorruption} {
diff --git a/internal/wasm/jit/mmap_test.go b/internal/wasm/jit/mmap_test.go
index 95d9387a..ea47c422 100644
--- a/internal/wasm/jit/mmap_test.go
+++ b/internal/wasm/jit/mmap_test.go
@@ -17,17 +17,32 @@ func Test_mmapCodeSegment(t *testing.T) {
 	assert.NoError(err)
 	// Verify that the mmap is the same as the original.
 	assert.Equal(code, newCode)
-
 	// TODO: test newCode can executed.
+
+	t.Run("panic on zero length", func(t *testing.T) {
+		require.PanicsWithError(t, "BUG: mmapCodeSegment with zero length", func() {
+			_, _ = mmapCodeSegment(make([]byte, 0))
+		})
+	})
 }
 
 func Test_munmapCodeSegment(t *testing.T) {
 	requireSupportedOSArch(t)
 	assert := require.New(t)
+
+	// Errors if never mapped
+	assert.Error(munmapCodeSegment(code))
+
 	newCode, err := mmapCodeSegment(code)
 	assert.NoError(err)
 	// First munmap should succeed.
 	assert.NoError(munmapCodeSegment(newCode))
 	// Double munmap should fail.
 	assert.Error(munmapCodeSegment(newCode))
+
+	t.Run("panic on zero length", func(t *testing.T) {
+		require.PanicsWithError(t, "BUG: munmapCodeSegment with zero length", func() {
+			_ = munmapCodeSegment(make([]byte, 0))
+		})
+	})
 }
diff --git a/internal/wasm/store_test.go b/internal/wasm/store_test.go
index a9be4b01..b3972fd9 100644
--- a/internal/wasm/store_test.go
+++ b/internal/wasm/store_test.go
@@ -307,19 +307,19 @@ func TestStore_Instantiate_Errors(t *testing.T) {
 }
 
 func TestStore_ExportImportedHostFunction(t *testing.T) {
-	m, err := NewHostModule("", map[string]interface{}{"host_fn": func(wasm.Module) {}})
+	m, err := NewHostModule("host", map[string]interface{}{"host_fn": func(wasm.Module) {}})
 	require.NoError(t, err)
 
 	s := newStore()
 
 	// Add the host module
-	_, err = s.Instantiate(context.Background(), m, "")
+	_, err = s.Instantiate(context.Background(), m, m.NameSection.ModuleName)
 	require.NoError(t, err)
 
 	t.Run("Module is the importing module", func(t *testing.T) {
 		_, err = s.Instantiate(context.Background(), &Module{
 			TypeSection:   []*FunctionType{{}},
-			ImportSection: []*Import{{Type: ExternTypeFunc, Name: "host_fn", DescFunc: 0}},
+			ImportSection: []*Import{{Type: ExternTypeFunc, Module: "host", Name: "host_fn", DescFunc: 0}},
 			MemorySection: &Memory{Min: 1},
 			ExportSection: map[string]*Export{"host.fn": {Type: ExternTypeFunc, Name: "host.fn", Index: 0}},
 		}, "test")
@@ -418,17 +418,20 @@ func newStore() *Store {
 	return NewStore(&mockEngine{shouldCompileFail: false, callFailIndex: -1}, Features20191205)
 }
 
-func (e *mockEngine) NewModuleEngine(_, _ []*FunctionInstance) (ModuleEngine, error) {
+// NewModuleEngine implements Engine.NewModuleEngine
+func (e *mockEngine) NewModuleEngine(_ string, _, _ []*FunctionInstance) (ModuleEngine, error) {
 	if e.shouldCompileFail {
 		return nil, fmt.Errorf("some compilation error")
 	}
 	return &mockModuleEngine{callFailIndex: e.callFailIndex}, nil
 }
 
+// FunctionAddress implements ModuleEngine.FunctionAddress
 func (e *mockModuleEngine) FunctionAddress(index Index) uintptr {
 	return uintptr(index)
 }
 
+// Call implements ModuleEngine.Call
 func (e *mockModuleEngine) Call(ctx *ModuleContext, f *FunctionInstance, _ ...uint64) (results []uint64, err error) {
 	if e.callFailIndex >= 0 && f.Index == Index(e.callFailIndex) {
 		return nil, fmt.Errorf("call failed")
@@ -437,7 +440,10 @@ func (e *mockModuleEngine) Call(ctx *ModuleContext, f *FunctionInstance, _ ...ui
 	return
 }
 
-func (e *mockModuleEngine) Close() {}
+// Close implements io.Closer
+func (e *mockModuleEngine) Close() (err error) {
+	return
+}
 
 func TestStore_getTypeInstance(t *testing.T) {
 	t.Run("too many functions", func(t *testing.T) {
diff --git a/tests/spectest/spec_test.go b/tests/spectest/spec_test.go
index 2109c017..1715d873 100644
--- a/tests/spectest/spec_test.go
+++ b/tests/spectest/spec_test.go
@@ -457,7 +457,7 @@ func requireInstantiationError(t *testing.T, store *wasm.Store, buf []byte, msg
 		return
 	}
 
-	_, err = store.Instantiate(context.Background(), mod, "")
+	_, err = store.Instantiate(context.Background(), mod, t.Name())
 	require.Error(t, err, msg)
 }
 
diff --git a/wasm_test.go b/wasm_test.go
index afac2088..42bd52c2 100644
--- a/wasm_test.go
+++ b/wasm_test.go
@@ -303,11 +303,11 @@ func TestRuntime_NewModule_UsesStoreContext(t *testing.T) {
 		require.Equal(t, runtimeCtx, ctx.Context())
 	}
 
-	_, err := r.NewModuleBuilder("").ExportFunction("start", start).Instantiate()
+	_, err := r.NewModuleBuilder("env").ExportFunction("start", start).Instantiate()
 	require.NoError(t, err)
 
 	decoded, err := r.CompileModule([]byte(`(module $runtime_test.go
-	(import "" "start" (func $start))
+	(import "env" "start" (func $start))
 	(start $start)
 )`))
 	require.NoError(t, err)