Merge pull request wasmerio#414 from miretskiy/memory

bugfix: Introduce GetFunctionSafe

go.mod

@@ -1,5 +1,11 @@
 module github.com/wasmerio/wasmer-go
 
-go 1.14
+go 1.22
 
 require github.com/stretchr/testify v1.7.0
+
+require (
+	github.com/davecgh/go-spew v1.1.0 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect
+)

wasmer/instance.go

@@ -19,13 +19,12 @@ type Instance struct {
 // Note:️ Instantiating a module may return TrapError if the module's
 // start function traps.
 //
-//   wasmBytes := []byte(`...`)
-//   engine := wasmer.NewEngine()
-//   store := wasmer.NewStore(engine)
-//   module, err := wasmer.NewModule(store, wasmBytes)
-//   importObject := wasmer.NewImportObject()
-//   instance, err := wasmer.NewInstance(module, importObject)
-//
+//	wasmBytes := []byte(`...`)
+//	engine := wasmer.NewEngine()
+//	store := wasmer.NewStore(engine)
+//	module, err := wasmer.NewModule(store, wasmBytes)
+//	importObject := wasmer.NewImportObject()
+//	instance, err := wasmer.NewInstance(module, importObject)
 func NewInstance(module *Module, imports *ImportObject) (*Instance, error) {
 	var traps *C.wasm_trap_t
 	externs, err := imports.intoInner(module)
@@ -87,6 +86,47 @@ func (self *Instance) SetRemainingPoints(newLimit uint64) {
 	C.wasmer_metering_set_remaining_points(self._inner, C.uint64_t(newLimit))
 }
 
+// ReleaseFn is a function to release resources
+// This function is parameterized to make sure that the correct
+// argument is passed to the function.
+type ReleaseFn[T any] func(T)
+
+func keepAlive[T any](value T) {
+	runtime.KeepAlive(value)
+}
+
+// GetFunctionSafe performs the same job as instance.Exports.GetFunction
+// but it returns a ReleaseFn to release the resources.
+//
+// The reason for this is that the following code *IS NOT SAFE*:
+//
+//	instance, err := NewInstance(module, NewImportObject())
+//	fn, err := instance.Exports.GetFunction("function_name")
+//	// No further usages of instance after this line.
+//	fn()
+//
+// The problem is that when fn() is called, it will issue CGo call to the
+// function.  When that happens, the instance may be garbage collected
+// because there are no longer any uses of instance.  The returned function
+// however *does* depended on the instance; we must ensure that instance
+// remains live as long long as the calls to *any* instance functions may happen.
+// One way to do that is to add the following line *immediately* after the
+// instance intialization:
+//
+//	defer runtime.KeepAlive(instance)
+//
+// Another mechanism is to use GetFunctionSafe which returns a ReleaseFn
+// which should be called if this function returns non-error value.
+// This usage is preferred because it is less error-prone (i.e. it
+// returns 3 values that should all be handled appropriately).
+func (self *Instance) GetFunctionSafe(name string) (NativeFunction, ReleaseFn[*Instance], error) {
+	fn, err := self.Exports.GetFunction(name)
+	if err != nil {
+		return nil, nil, err
+	}
+	return fn, keepAlive, nil
+}
+
 // Force to close the Instance.
 //
 // A runtime finalizer is registered on the Instance, but it is

wasmer/memory_test.go

@@ -1,10 +1,17 @@
 package wasmer
 
 import (
-	"github.com/stretchr/testify/assert"
+	"embed"
+	"runtime"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+//go:embed testdata
+var testData embed.FS
+
 func TestMemory(t *testing.T) {
 	engine := NewEngine()
 	store := NewStore(engine)
@@ -86,3 +93,59 @@ func TestMemoryData(t *testing.T) {
 	assert.Equal(t, "Aello, World!", string(data2[pointer:pointer+13]))
 
 }
+
+// This test exercises proper memory management.
+// To do so, we run many iterations of "sum" function, while
+// invoking GC.
+//
+// Errors observed prior to this fix include: SIGSEGV, SIGBUS as
+// well as simply test failure, followed by panic, where the sum
+// function points to an incorrect function
+// (Parameters of type [I32, I32] did not match signature [] -> [])
+//
+// https://github.com/wasmerio/wasmer-go/issues/391
+// https://github.com/wasmerio/wasmer-go/issues/364
+func TestSumLoop(t *testing.T) {
+	//debug.SetGCPercent(1) -- This also reproduces the issue,
+	//but having explicit runtime.GC call below does that too.
+	e := NewEngineWithConfig(NewConfig().UseCraneliftCompiler())
+	s := NewStore(e)
+
+	src, err := testData.ReadFile("testdata/sum.wasm")
+	require.NoError(t, err)
+
+	mod, err := NewModule(s, src)
+	require.NoError(t, err)
+
+	// Configure WASI_VERSION_SNAPSHOT1 environment
+	we, err := NewWasiStateBuilder(mod.Name()).
+		CaptureStdout().
+		CaptureStderr().
+		Finalize()
+	require.NoError(t, err)
+
+	imp, err := we.GenerateImportObject(s, mod)
+	require.NoError(t, err)
+
+	// Let's instantiate the WebAssembly module.
+	instance, err := NewInstance(mod, imp)
+	require.NoError(t, err)
+
+	sum, release, err := instance.GetFunctionSafe("sum")
+	require.NoError(t, err)
+	defer release(instance)
+
+	// This causes the issue to reproduce on the very first iteration
+	// if KeepAlive call removed.
+	runtime.GC()
+
+	hi := 10240
+	n := int32(0)
+	for i := range hi {
+		res, err := sum(n, i+1)
+		//runtime.GC()
+		require.NoError(t, err)
+		n = res.(int32)
+	}
+	require.EqualValues(t, hi*(hi+1)/2, n)
+}