From a4ecc770532aceefed6b7531647c3353e78be028 Mon Sep 17 00:00:00 2001 From: Yevgeniy Miretskiy Date: Wed, 8 Jan 2025 21:44:07 -0500 Subject: [PATCH] bugfix: Introduce GetFunctionSafe Fixes https://github.com/wasmerio/wasmer-go/issues/364 Fixes https://github.com/wasmerio/wasmer-go/issues/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 Release note (): --- go.mod | 8 ++++- wasmer/instance.go | 54 +++++++++++++++++++++++++++----- wasmer/memory_test.go | 65 ++++++++++++++++++++++++++++++++++++++- wasmer/testdata/sum.wasm | Bin 0 -> 9101 bytes 4 files changed, 118 insertions(+), 9 deletions(-) create mode 100644 wasmer/testdata/sum.wasm 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 0000000000000000000000000000000000000000..9a212a544d619fa8d78f9504c07896e7f7af8d48 GIT binary patch literal 9101 zcmZ`z22E-A*LB|19ynvN;RlqgFc%8Co~D3N5>6aFxo7<2m-e?TBAsTF47_?vT%!_=psP7NP#Z0$igVP$fAoZvM}I& z-+%6mC?%FXJpXz9=Rg1ZKQpR3+}27deZKlybcBEU=rtpM^^qc^UQ-y-9G7|Ym3c}( zcE3B^SRL+k_lEa&N2_~--r+{?{-ye$yK{GUdv(1xQre!HHa1c!)`?L@>xwmzj$*y1 zt&Oc#CaD6iKR%AqimxQwz3tt>n?@zu-L0+NHI+oK4|+XSOV&caQ5p0C)J#`5b~Z*E z-K~v3?5UO=9&D@WT2(8rQxnAvZR`K@;`A|&JbF_3m42b;mDB!e|GHYXgl7BxC&$q_ zRd!w9|MWz6q3?h8pvyf=*z+!iEZ{0&kSs89{kUA3IrK8atxn+tVpocY(apwP9E)ys z+{I?6!pg86lPjCV1K?QqOX%YXbZjPL6X@7Z#!5&7$-GK`k?5z7{I~K`PA{0On8_;y zRPlv8Ca_dImL~*U@x>h1S}-%kY;Fid#p8L6z?I^3o)VZZ&g5+Zv&CF)2~>+G@&H;Dw^dn*^RIp3GZz#YL`(&s4vNhCaJrG+nD-w1&QM zznFFrMqR}vE_RiEQIQ3)tM-dT7F0Pp@8Y4a-Y;U8VBA%QzH-07c-2{#;nV0BRyHQfFMPUl6t2V zWt?ntrg9%lPH;Z%C;Q}L)=dk2xL87bxG;9t9PSQO5wFnT6>y)Fq*OTrR1pxxpswz| zOiG>-B)9{v4iwl`ceo@_U>Wg&KR}W2#x2eOg~j%!d)h6?UgPG+yRl~4;m+7?xI1=W z5_UJ;H1T2vyNx^L!Gu$O`sJpA9=N1qU<@wq7-+kzbPNRVsvYCdn2wo2lXlD%G_{U_ z3b=O1%%ia#Gm9qcn5$?S9Rq!H^^RFU6LriSnr6qmfM%*=pjEEbG0(sxny%<~)C+(z z_a#7A0j2IVpeuk9_ZXlhKrQzqAnvHUvw#)=wcQy&^MI=Ei+~)U$T>iBfUKJXGz-YM zSwJ&@DsBc4EqTi2fEs`r?g>CuKsEO`AQ&uU@&!QT05p2Z>pr-W{&hpA^f%dNQH8&$ z`bIym`zTNRkB%dl-Hm94&ceG)`OiMH`-Q^54~~Hk%uKoH`X3+T-^Xg1{f}i+uI!3g zp)-hXMk_8s`)afT`>8<486<##K2;9wW|8>M>?;sd>XI8x?LRq=GEGFDM|hP(gxm+c zQk?AP#;lkl;OBOyHVfsz)K|_Fabdl?jRTFh`!}4uF=#?g&MYc*O@WKJGNv>$8I<`9(!(Zn3}8z$vh z&VE`~iXF>&PNpCp>@{FGt_Fd;jEkt;KtAkk7RG1$n1l-GbB5dwP<5GF311B)mJMu& zEW>hWKQVysziE$*Wy`Pyfo7MX>()CW&}?8D-tpLW5RUOx2=OE;LmKN^|W`;b2m2m-^+A?P4#7Uc!5q%nzJ@VOzOgtQP3?hY}hBEmUV})<^7qrUJLJk%a4MC-SEWysi zV+6BAP$dG5T~kER6a>xwf+B*pAef?r%@Gfy{44_#B{;d1;8VI2=sc(5WhG}^RL!rb zW?0w0rrIC|vLvxtKNZ!l?l(=KjyItaK78d$ptvF@;L24}JWGnnkW-|%Eflv)ikm|5 zDWE5+8p1J68p!KjwvAYOX4T-wg=M=N@@K&FuN;JU5+=L<8Fvby6V7Q?hdn-!L9+M9=8uZw5n(8rL zSeYz@fxs@ua3E{Rf+`Sl+W1BZY5#bM zno&d+!dr6{Ts4~p7H5-fGZScb0?mz4VMDa!&%rjX5?MRI@`{t_k5Dv0FQnIfjCw25pUbjvNF^wbhE$|pp`{11ja(nlkh#%Ad*Y#V_4*#pS zBYrRd)LH-UW$+lQ@Ck!w`#w4>YGLO8-sU9>lUuujTS1h!V3@K0Oz*?D=sYH$`Jang zCQ@1`!;aHn7iy2J&A!Ij7UFW;dvAd$ZaLkS$HU4fESI44#*nSv{bR<5hO z)rhEu23nUuxy_*{8r;IjC;NxcA#a&?m@J!Yot(%(M1yuQtSBwd2LxZ^O&(`cUc=GC z501|wDV)T4{ykw#-X;wg0C{@J z92+saHsn&YAsM=P;*M@K?V~H1G;p&yu^B5lT!9dUR@VY1Pzb}sJ<1D1SZV6t2T$># zcv~WG+}XAM)<+|F0Ay4U5B%WHt4)Ug^>qFFt5n}7VVT#(JTK;&23TX0g7p$6ocfwX!?jLk6QaM?Q07#Oh}TjnqOY*cgox2Bj)C3&)Va&iMa@hroxV;jsfMGDNEn zLt6`dGBF_{p%*1PuhQsyJA&VG_^WU~xtN^ea4Hwba7iKhne7tf!?avIYSM9nqr5(HaZYiP@&U2Utv*FqXRU%GF{%m_y1x&FuDL0C*cKMoA?d#G z=+P|?kjiGq&36N$`2vOWJ=Y`y!ZODaxrI-ixbVp0no~+;b+}_V6(l53FFyKU-&TO< z3zXt}u1+VJl$)^xip>2@SLgmJ?o{qoa}}CK4g-au;)M#Hh31FyoE8bcMe~ECBpGxGOay4z=R zXkcEEz#r2^i`l}NFi#-EI_Bf@8b4+W4*UpjF>oY!YG7E6WtoKpc{#iR#W=VP&BDW2 z1N!OgfJzNE{I~3fO;N`?=0DjL$xl>J_GX;Nu|L&j#R-NQoIlnA|16c}XWWR+T!A;8 zpW|icMiWnpczS6;`x@Y?C>>vKkPX84rC2A2KkqUj#8eq#u&7en^O$p=@VJLK$VDCV zeU9T={~-71nD4R0y*>hah{;0l<$V4bdsO;K3Y}v#d%A4P3=yxRvaZl#ujmR_9^8OM z9z$v6M$^D6;b0oaBbZ7gf&#gf9507pGIh9f$xk8)XQF_Av=( zcn;SS@J&2H1~``ch3GqBFwH>%c15hA@{lz!ghG@Ip=f3tHU1HEs6(G) zDmOzHYyiAql0r?cQx7RjItVCMPIUn=OGU&*2#Xh*N1$u67+^V{Y#}p)*4G|rCIy&? zd|@}3CNhc%DW7dXQ!O>p`msiiBTPyC!lRdiVtOdRkt36z;3Amkmsnuf z^oKUQ8bTrvw$nil`XM4AeQsftr;L1%s-GkF$Jr_&`P` zi3Hu0|L~aQ!bv|xFNIBfl10O63QbvC&4Bd=Qh!uJa8{5JO7*_@Dl6RzD;m5<13yEX z_8$d`;4d6{-wQnmeV#88unb8)BOFg5)?FN=(~Qb1GWUK+44{mT;Q^H;&0}JnX=-Ht zFeS!xS}KH5B4d&{F&xC(k~sy0Awe4QV*cviKSp{llp=S5$b#_T3n3&CUfP+MshO8l z6_$& zHpL-gu`ZF}(1yg1bo{;nc}QN%pqvO^P^|;*z!6aoH%Mkml8a6 zXo%2*^C;%QniCph1U=lNwIIdgI3D>55AMZV;j^9g^`FJp(qGmR0trrZK#HbSm3)9?Ubv$ z<6^LW>kQdXy|k{y3=IT9JjScvrjl;L%wAM0Aoo^bqLmj_E~ZIMu(VN7eZt5&tbfhg zKiAeB1+O>hmT+QPfw1vxo2J(-i~_$P$a-(=;kO6w^0SL~-W>JZCLV$?Y(FwuJb(Il zr0_ivJ{lkX8@ERN%0xTe?VgIG?e4~o{-HUgo>^Vp*xuV6jCyxhfAdcrRv-M-K{wX! zb@BhRORIyOb<>{o4)*SLM?G8b4%V)Oj&NSp0QuO zbV=E-KL5OGV0x`L9QN))t|ngNl5bwRdg0QAXWhcR(P(dY?c&AtjnTb>I~Ue=w=a%1 zcHUgyUECe4U)