diff --git a/go.mod b/go.mod index 116c9e05..b8a06ac9 100644 --- a/go.mod +++ b/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 +) diff --git a/wasmer/instance.go b/wasmer/instance.go index 96de2478..a6e5ed2a 100644 --- a/wasmer/instance.go +++ b/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 diff --git a/wasmer/memory_test.go b/wasmer/memory_test.go index 01b052ba..0049ecff 100644 --- a/wasmer/memory_test.go +++ b/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) +} diff --git a/wasmer/testdata/sum.wasm b/wasmer/testdata/sum.wasm new file mode 100644 index 00000000..9a212a54 Binary files /dev/null and b/wasmer/testdata/sum.wasm differ