Skip to content

Commit 812f47a

Browse files
Yevgeniy Miretskiymiretskiy
Yevgeniy Miretskiy
authored and
miretskiy
committed
bugfix: Tighten up memory model.
Fixes #415 As documented in #415, the memory mode exposed by wasmer-go is error prone and hard to use. Introduce a `CPtrBase[T]` helper struct to attempt to surface errors often and early. The `CPtrBase` is meant to store a CGo pointer. Then, through conditional compilation, a debug implementation, enabled via `-tags memcheck` tag, is used, which adds expensive pointer access checks as well as forces calls to `runtime.GC()` whenever underlying pointer accessed. These access are very slow -- however, they cause almost immediate crashes to surface in tests. It's possible that some issues still remain; The fixes were applied in order to ensure that the tests pass 5000 times. Beyond that, there could still be remaining issues.
1 parent 50a7979 commit 812f47a

27 files changed

+554
-201
lines changed

‎wasmer/config.go

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -87,22 +87,18 @@ func IsEngineAvailable(engine EngineKind) bool {
8787

8888
// Config holds the compiler and the Engine used by the Store.
8989
type Config struct {
90-
_inner *C.wasm_config_t
90+
CPtrBase[*C.wasm_config_t]
9191
}
9292

9393
// NewConfig instantiates and returns a new Config.
9494
//
9595
// config := NewConfig()
9696
func NewConfig() *Config {
97-
config := C.wasm_config_new()
98-
99-
return &Config{
100-
_inner: config,
101-
}
97+
return &Config{CPtrBase: mkPtr(C.wasm_config_new())}
10298
}
10399

104100
func (self *Config) inner() *C.wasm_config_t {
105-
return self._inner
101+
return self.ptr()
106102
}
107103

108104
// UseUniversalEngine sets the engine to Universal in the configuration.
@@ -190,7 +186,9 @@ func metering_delegate(op C.wasmer_parser_operator_t) C.uint64_t {
190186
// I32Add: 4,
191187
// }
192188
// config.PushMeteringMiddleware(7865444, opmap)
193-
func (self *Config) PushMeteringMiddleware(maxGasUsageAllowed uint64, opMap map[Opcode]uint32) *Config {
189+
func (self *Config) PushMeteringMiddleware(
190+
maxGasUsageAllowed uint64, opMap map[Opcode]uint32,
191+
) *Config {
194192
if opCodeMap == nil {
195193
// REVIEW only allowing this to be set once
196194
opCodeMap = opMap
@@ -278,7 +276,7 @@ func (self *Config) UseSinglepassCompiler() *Config {
278276
// config := NewConfig()
279277
// config.UseTarget(target)
280278
func (self *Config) UseTarget(target *Target) *Config {
281-
C.wasm_config_set_target(self.inner(), target.inner())
279+
C.wasm_config_set_target(self.inner(), target.release())
282280

283281
return self
284282
}

‎wasmer/config_test.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,16 @@ func TestConfig(t *testing.T) {
2929
engine := NewEngineWithConfig(config)
3030
store := NewStore(engine)
3131
module, err := NewModule(store, testGetBytes("tests.wasm"))
32+
defer runtime.KeepAlive(module)
33+
3234
assert.NoError(t, err)
3335

3436
instance, err := NewInstance(module, NewImportObject())
3537
assert.NoError(t, err)
3638

37-
sum, err := instance.Exports.GetFunction("sum")
39+
sum, release, err := instance.GetFunctionSafe("sum")
3840
assert.NoError(t, err)
41+
defer release(instance)
3942

4043
result, err := sum(37, 5)
4144
assert.NoError(t, err)
@@ -53,12 +56,14 @@ func TestConfigForMetering(t *testing.T) {
5356
store := NewStore(engine)
5457
module, err := NewModule(store, testGetBytes("tests.wasm"))
5558
assert.NoError(t, err)
59+
defer runtime.KeepAlive(module)
5660

5761
instance, err := NewInstance(module, NewImportObject())
5862
assert.NoError(t, err)
5963

60-
sum, err := instance.Exports.GetFunction("sum")
64+
sum, release, err := instance.GetFunctionSafe("sum")
6165
assert.NoError(t, err)
66+
defer release(instance)
6267

6368
result, err := sum(37, 5)
6469
assert.NoError(t, err)
@@ -74,12 +79,14 @@ func TestConfigForMeteringFn(t *testing.T) {
7479
store := NewStore(engine)
7580
module, err := NewModule(store, testGetBytes("tests.wasm"))
7681
assert.NoError(t, err)
82+
defer runtime.KeepAlive(module)
7783

7884
instance, err := NewInstance(module, NewImportObject())
7985
assert.NoError(t, err)
8086

81-
sum, err := instance.Exports.GetFunction("sum")
87+
sum, release, err := instance.GetFunctionSafe("sum")
8288
assert.NoError(t, err)
89+
defer release(instance)
8390

8491
result, err := sum(37, 5)
8592
assert.NoError(t, err)
@@ -144,12 +151,14 @@ func TestConfig_AllCombinations(t *testing.T) {
144151
store := NewStore(engine)
145152
module, err := NewModule(store, testGetBytes("tests.wasm"))
146153
assert.NoError(t, err)
154+
defer runtime.KeepAlive(module)
147155

148156
instance, err := NewInstance(module, NewImportObject())
149157
assert.NoError(t, err)
150158

151-
sum, err := instance.Exports.GetFunction("sum")
159+
sum, release, err := instance.GetFunctionSafe("sum")
152160
assert.NoError(t, err)
161+
defer release(instance)
153162

154163
result, err := sum(37, 5)
155164
assert.NoError(t, err)

‎wasmer/cptr.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package wasmer
2+
3+
// CPtrBase is a based struct for a C pointer.
4+
// It is intended to be embedded into any structure
5+
// that stores a C pointer.
6+
// While this based is parameterized on type any, using any
7+
// type other than *C.xxx pointer should be considered an undefined behavior.
8+
type CPtrBase[T comparable] struct {
9+
_ptr T
10+
maybeStack // stack of the creating goroutine (if enabled via memcheck)
11+
maybeNil[T] // indicates if initial value of _ptr was nil (if enabled via memcheck)
12+
}
13+
14+
// release returns the C pointer stored in this base and clears the finalizer.
15+
func (b *CPtrBase[T]) release() T {
16+
var zero T
17+
v := b.ptr()
18+
b._ptr = zero
19+
b.ClearFinalizer()
20+
return v
21+
}
22+
23+
func (b *CPtrBase[T]) SetFinalizer(free func(v T)) {
24+
doSetFinalizer(b, free)
25+
}
26+
27+
func (b *CPtrBase[T]) ClearFinalizer() {
28+
doClearFinalizer(b)
29+
}
30+
31+
func mkPtr[T comparable](ptr T) CPtrBase[T] {
32+
return doMkPtr(ptr)
33+
}

‎wasmer/cptr_check.go

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
//go:build memcheck
2+
3+
package wasmer
4+
5+
import (
6+
"bytes"
7+
"fmt"
8+
"os"
9+
"os/signal"
10+
"runtime"
11+
"sync"
12+
"sync/atomic"
13+
"syscall"
14+
"unsafe"
15+
)
16+
17+
func doMkPtr[T comparable](ptr T) CPtrBase[T] {
18+
var zero T
19+
b := CPtrBase[T]{_ptr: ptr}
20+
b._stack = stack(3)
21+
b.maybeNil._nil = ptr == zero
22+
b.maybeNil._zero = zero
23+
return b
24+
}
25+
26+
type maybeNil[T comparable] struct {
27+
_nil bool
28+
_zero T
29+
}
30+
31+
// maybeStack stores the stack of the goroutine.
32+
type maybeStack struct {
33+
_stack []byte
34+
}
35+
36+
// ptr returns the C pointer stored in this base.
37+
func (b *CPtrBase[T]) ptr() T {
38+
if !b.maybeNil._nil && b._ptr == b.maybeNil._zero {
39+
panic(fmt.Errorf("attempt to use a released object; ptr was allocated by\n%s", b._stack))
40+
}
41+
runtime.GC()
42+
return b._ptr
43+
}
44+
45+
func doSetFinalizer[T comparable](b *CPtrBase[T], free func(v T)) {
46+
finalizers.Store(uintptr(unsafe.Pointer(b)), stack(3))
47+
48+
runtime.SetFinalizer(b, func(b *CPtrBase[T]) {
49+
ptr := b._ptr
50+
b._ptr = b.maybeNil._zero
51+
b.maybeNil._nil = false
52+
free(ptr)
53+
finalizers.Delete(uintptr(unsafe.Pointer(b)))
54+
})
55+
}
56+
57+
func doClearFinalizer[T comparable](b *CPtrBase[T]) {
58+
runtime.SetFinalizer(b, nil)
59+
finalizers.Delete(uintptr(unsafe.Pointer(b)))
60+
}
61+
62+
var finalizers sync.Map // uintptr -> []byte (the stack of the caller which created the finalizer)
63+
64+
func init() {
65+
c := make(chan os.Signal, 1)
66+
signal.Notify(c, syscall.SIGABRT, syscall.SIGSEGV, syscall.SIGBUS, syscall.SIGINT)
67+
68+
go func() {
69+
s := <-c
70+
fmt.Fprintf(os.Stderr, "Signal %s received\n", s)
71+
72+
allStacks := make([]byte, 1<<18)
73+
sl := runtime.Stack(allStacks, true)
74+
if sl == len(allStacks) {
75+
// Try again, once, with a bigger buffer
76+
allStacks = make([]byte, 1<<20)
77+
sl = runtime.Stack(allStacks, true)
78+
}
79+
allStacks = allStacks[:sl]
80+
81+
fmt.Fprintf(os.Stderr, "\n%s\n\n", allStacks)
82+
fmt.Fprintf(os.Stderr, "Pending Finalizers:\n")
83+
84+
n := 0
85+
finalizers.Range(func(k, v interface{}) bool {
86+
n++
87+
stack := v.([]byte)
88+
fmt.Fprintf(os.Stderr, "\n--finilizer %d: %s\n\n", n, stack)
89+
return true
90+
})
91+
os.Exit(1)
92+
}()
93+
}
94+
95+
// stackCache seems to be a bit of an overkill for debug build, but
96+
// some of the tests (e.g. TestSumLoop) allocate many pointers; and
97+
// the time and memory used for that adds up.
98+
const stackCacheMaxSize = 1 << 9
99+
100+
var stackCache sync.Map // uintptr -> []byte
101+
var stackCacheSize atomic.Int32
102+
103+
func stack(skip int) []byte {
104+
var buf bytes.Buffer
105+
const maxDepth = 32
106+
pc := make([]uintptr, maxDepth)
107+
// Skip the requested number of frames + the frames for this function
108+
n := runtime.Callers(skip+1, pc)
109+
if n == 0 {
110+
return nil
111+
}
112+
pc = pc[:n]
113+
114+
if v, ok := stackCache.Load(pc[0]); ok {
115+
return v.([]byte)
116+
}
117+
118+
// Convert program counters to frames
119+
frames := runtime.CallersFrames(pc)
120+
for {
121+
frame, more := frames.Next()
122+
// Format the frame as "file:line function"
123+
fmt.Fprintf(&buf, "%s:%d %s\n", frame.File, frame.Line, frame.Function)
124+
if !more {
125+
break
126+
}
127+
}
128+
129+
s := buf.Bytes()
130+
stackCache.Store(pc[0], s)
131+
if stackCacheSize.Add(1) > stackCacheMaxSize {
132+
stackCache.Range(func(k, v interface{}) bool {
133+
stackCache.Delete(k)
134+
return stackCacheSize.Add(-1) > (stackCacheMaxSize >> 1)
135+
})
136+
}
137+
return s
138+
}

‎wasmer/cptr_nocheck.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
//go:build !memcheck
2+
3+
package wasmer
4+
5+
import "runtime"
6+
7+
func doMkPtr[T comparable](ptr T) CPtrBase[T] {
8+
return CPtrBase[T]{_ptr: ptr}
9+
}
10+
11+
// maybeStack is a no-op implementation for storing
12+
// ptr creator stack.
13+
type maybeStack struct{}
14+
15+
// maybeNil is a no-op implementation for indicating
16+
// if initial value of _ptr was nil.
17+
type maybeNil[T comparable] struct{}
18+
19+
// ptr returns the C pointer stored in this base.
20+
func (b *CPtrBase[T]) ptr() T {
21+
return b._ptr
22+
}
23+
24+
func doSetFinalizer[T comparable](b *CPtrBase[T], free func(v T)) {
25+
runtime.SetFinalizer(b, func(b *CPtrBase[T]) {
26+
free(b.ptr())
27+
})
28+
}
29+
30+
func doClearFinalizer[T comparable](b *CPtrBase[T]) {
31+
runtime.SetFinalizer(b, nil)
32+
}

‎wasmer/engine.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,18 @@ package wasmer
22

33
// #include <wasmer.h>
44
import "C"
5-
import "runtime"
65

76
// Engine is used by the Store to drive the compilation and the
87
// execution of a WebAssembly module.
98
type Engine struct {
10-
_inner *C.wasm_engine_t
9+
CPtrBase[*C.wasm_engine_t]
1110
}
1211

1312
func newEngine(engine *C.wasm_engine_t) *Engine {
14-
self := &Engine{
15-
_inner: engine,
16-
}
17-
18-
runtime.SetFinalizer(self, func(self *Engine) {
19-
C.wasm_engine_delete(self.inner())
13+
self := &Engine{CPtrBase: mkPtr(engine)}
14+
self.SetFinalizer(func(v *C.wasm_engine_t) {
15+
C.wasm_engine_delete(v)
2016
})
21-
2217
return self
2318
}
2419

@@ -58,7 +53,7 @@ func NewDylibEngine() *Engine {
5853
}
5954

6055
func (self *Engine) inner() *C.wasm_engine_t {
61-
return self._inner
56+
return self.ptr()
6257
}
6358

6459
// NewJITEngine is a deprecated function. Please use NewUniversalEngine instead.

‎wasmer/engine_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,16 @@ import (
99

1010
func testEngine(t *testing.T, engine *Engine) {
1111
store := NewStore(engine)
12-
module, err := NewModule(store, testGetBytes("tests.wasm"))
12+
module, modrelease, err := NewModuleSafe(store, testGetBytes("tests.wasm"))
1313
assert.NoError(t, err)
14+
defer modrelease(module)
1415

1516
instance, err := NewInstance(module, NewImportObject())
1617
assert.NoError(t, err)
1718

18-
sum, err := instance.Exports.GetFunction("sum")
19+
sum, release, err := instance.GetFunctionSafe("sum")
1920
assert.NoError(t, err)
21+
defer release(instance)
2022

2123
result, err := sum(37, 5)
2224
assert.NoError(t, err)
@@ -44,7 +46,6 @@ func TestEngineWithTarget(t *testing.T) {
4446
assert.NoError(t, err)
4547

4648
cpuFeatures := NewCpuFeatures()
47-
assert.NoError(t, err)
4849

4950
target := NewTarget(triple, cpuFeatures)
5051

0 commit comments

Comments
 (0)