bugfix: Introduce GetFunctionSafe

Fixes wasmerio#364
Fixes wasmerio#391

It's a very common pattern to save exported function for
the purpose of future invocation:
```
  fn, err := instance.Exports.GetFunction("...")
  ...
  return fn(...)
```

Such pattern of use is bound to run into issues when the
code will crash occasionally when invoking fn.
This happens *if* after the function instantiation the instance
is never used/accessed again.  In those cases, Go GC may
GC the instance object -- and if that happens the call to fn
will likely panic.

A safer version of GetFunction method has been added on the
instance:
```
  fn, release, err := instance.GetFunctionSafe(...)
  // handle err
  defer release(instance)
```

By returning release function that must be called with the owning
instance object, we ensure the instance remains live as long as the
instance function may be called.  Indeed, the release function is
nothing more than a typed runtime.KeepAlive call.

Signed-off-by: Yevgeniy Miretskiy <[email protected]>

Release note (<category, see below>): <what> <show> <why>

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)
+}