Repository navigation
fix(wasm): double-free of guest-owned Vec params corrupted the WASM heap - #3
Conversation
The wasm-bindgen reference-types ABI transfers ownership of Vec<String> / Vec<Vec<u8>> argument arrays to the guest: during the call the guest converts every element, recycles the externref table slots and frees the array buffer itself. The wrapper's deferred freeStringArray / freeUint8ArrayArray then operated on freed memory — reading garbage table indices, re-deallocing guest slots (poisoning the slot free-list) and double-freeing the array buffer — corrupting the dlmalloc heap. The next malloc-heavy export then trapped OOB (encode_partially_signed_transaction, encode_witness_no_signature, ..., wherever the damaged chunk was hit first). Affected exports: - estimate_transaction_size (EstimateTransactionSize) — hit by the fee fixed-point loop up to 3x before the first PST encode, which is why the conclude+create flow was the live incident - encode_signed_transaction_intent (EncodeSignedTransactionIntent) - verify_transaction_intent (VerifyTransactionIntent) Fix: mirror wasm-bindgen's own JS glue for owned-Vec params. The array writers now return an externRefArray that tracks what the wrapper still owns; post-call release() frees only the Go refs entries (plus the wrapper-owned element backing buffers for byte arrays, which the guest only copies from, never frees) and never touches the guest-owned array buffer or re-reads indices from it. discard() handles error paths where the export was never invoked and the wrapper still owns everything. Regression tests (wasm/ownership_regression_test.go) reproduce the corruption deterministically on the old code — estimate x3 then encode canary traps OOB, intent-encode canary yields mangled heap reads — and pass after the fix. Full suite green, including repeat runs.
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (a *externRefArray) release(c *Client) { | ||
| for i, key := range a.keys { | ||
| if i < len(a.elems) { | ||
| c.freeWASM(a.elems[i].ptr, a.elems[i].len) | ||
| } | ||
| refs.free(key) | ||
| } | ||
| } |
There was a problem hiding this comment.
release/discard are not idempotent, and the new caller pattern in transactions.go triggers a double free: EstimateTransactionSize installs defer dests.release(c) and then, on a writeBytes error, calls dests.discard(c) before returning. discard internally calls release (freeing refs keys and element buffers), and the deferred release then frees the same keys/buffers a second time — refs.free on already-freed keys and a double freeWASM of the wrapper-owned byte-array backing buffers, corrupting the refs registry and the WASM heap. Make release (and discard) safe against double invocation, e.g. by zeroing keys/elems/ptr/count after releasing, and re-check all call sites that combine discard with a deferred release.
Suggestion:
| func (a *externRefArray) release(c *Client) { | |
| for i, key := range a.keys { | |
| if i < len(a.elems) { | |
| c.freeWASM(a.elems[i].ptr, a.elems[i].len) | |
| } | |
| refs.free(key) | |
| } | |
| } | |
| func (a *externRefArray) release(c *Client) { | |
| for i, key := range a.keys { | |
| if i < len(a.elems) { | |
| c.freeWASM(a.elems[i].ptr, a.elems[i].len) | |
| } | |
| refs.free(key) | |
| } | |
| a.keys = nil | |
| a.elems = nil | |
| a.ptr = 0 | |
| a.count = 0 | |
| } |
| func (a *externRefArray) discard(c *Client) { | ||
| a.release(c) | ||
| c.freeWASM(a.ptr, a.count*4) | ||
| } |
There was a problem hiding this comment.
On pre-invocation error paths the export never runs, so the guest never recycles the externref table slots; discard (and release) frees the Go refs keys and buffers but never calls __externref_table_dealloc. The removed freeStringArray/freeUint8ArrayArray did deallocate slots, so this change permanently leaks table slots on every error path (e.g., writeBytes failure for outputs in EstimateTransactionSize), which can exhaust the externref table over repeated failures. Capture the allocated table indices and call __externref_table_dealloc for each in discard.
Suggestion:
| func (a *externRefArray) discard(c *Client) { | |
| a.release(c) | |
| c.freeWASM(a.ptr, a.count*4) | |
| } | |
| func (a *externRefArray) discard(c *Client) { | |
| deallocFn := c.mod.ExportedFunction("__externref_table_dealloc") | |
| for _, idx := range a.tableIdxs { | |
| if deallocFn != nil { | |
| deallocFn.Call(c.ctx, uint64(idx)) //nolint:errcheck | |
| } | |
| } | |
| a.release(c) | |
| c.freeWASM(a.ptr, a.count*4) | |
| } |
| defer dests.release(c) | ||
| outPtr, outLen, err := c.writeBytes(outputs) | ||
| if err != nil { | ||
| dests.discard(c) // export never called: wrapper still owns the buffer |
There was a problem hiding this comment.
On the writeBytes(outputs) error path, discard(c) runs while the deferred dests.release(c) (registered two lines above) is still pending, so the array is cleaned up twice. This is currently harmless only because string arrays have empty elems and refs.free is an idempotent map delete; if this pattern is copied for writeUint8ArrayArray results (or refs.free later frees associated buffers), it becomes a genuine double free of the wrapper-owned element buffers. Safer to discard before/without the deferred release, e.g. move the defer registration after the outputs write, or have the error path return early with only discard and no prior defer.
Suggestion:
| defer dests.release(c) | |
| outPtr, outLen, err := c.writeBytes(outputs) | |
| if err != nil { | |
| dests.discard(c) // export never called: wrapper still owns the buffer | |
| outPtr, outLen, err := c.writeBytes(outputs) | |
| if err != nil { | |
| dests.discard(c) // export never called: wrapper still owns the buffer | |
| return 0, err | |
| } | |
| defer dests.release(c) |
Address all four findings from the OpenCodeReview run 35985298671: - critical (bug): release()/discard() are now idempotent (they zero the record after cleanup), so a discard followed by the deferred release can never free refs/element buffers twice. - medium (maintainability): EstimateTransactionSize no longer registers the deferred release before the outputs writeBytes error path; discard now runs alone on that path (no overlap with a pending release). - medium (bug): discard() now recycles the externref table slots via __externref_table_dealloc (single i32 slot index, verified against the module export signature) using table indices captured at write time, instead of permanently leaking slots on pre-invocation error paths. - low (maintainability): writeUint8ArrayArray writes element indices at offset i (loop index), consistent with writeStringArray, instead of len(arr.keys)-1.
| var buf [4]byte | ||
| binary.LittleEndian.PutUint32(buf[:], tableIdx) | ||
| c.mod.Memory().Write(arrPtr+uint32(len(tableIndices)-1)*4, buf[:]) | ||
| c.mod.Memory().Write(arr.ptr+uint32(i)*4, buf[:]) | ||
| } | ||
| return arrPtr, n, nil | ||
| return arr, nil | ||
| } |
There was a problem hiding this comment.
The error return of c.mod.Memory().Write is ignored when writing each table index into the array buffer. A failed Write (e.g. out-of-bounds) leaves that slot uninitialized while the function still returns success with count=n; the guest would then read a garbage externref index during element conversion. Other write paths in this file already check the result (see writeBytes at line ~286: if !c.mod.Memory().Write(ptr, data) {). Check the result and abort with arr.discard(c) plus an error.
Suggestion:
| var buf [4]byte | |
| binary.LittleEndian.PutUint32(buf[:], tableIdx) | |
| c.mod.Memory().Write(arrPtr+uint32(len(tableIndices)-1)*4, buf[:]) | |
| c.mod.Memory().Write(arr.ptr+uint32(i)*4, buf[:]) | |
| } | |
| return arrPtr, n, nil | |
| return arr, nil | |
| } | |
| var buf [4]byte | |
| binary.LittleEndian.PutUint32(buf[:], tableIdx) | |
| if !c.mod.Memory().Write(arr.ptr+uint32(i)*4, buf[:]) { | |
| arr.discard(c) | |
| return externRefArray{}, fmt.Errorf("mintlayer: write table index %d for array: out of bounds", i) | |
| } | |
| } | |
| return arr, nil |
| dests, err := c.writeStringArray(inputUtxosDests) | ||
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
The inputs buffer leaks on this error path. inPtr/inLen were allocated by writeBytes(inputs) and are only freed by the guest during the export; since writeStringArray failed here, the export is never invoked and nothing frees inPtr. The wrapper still owns it on this pre-export path and must free it before returning (e.g. c.freeWASM(inPtr, inLen)), mirroring the dests.discard(c) handling below.
Suggestion:
| dests, err := c.writeStringArray(inputUtxosDests) | |
| if err != nil { | |
| return 0, err | |
| } | |
| dests, err := c.writeStringArray(inputUtxosDests) | |
| if err != nil { | |
| c.freeWASM(inPtr, inLen) // export never called: wrapper still owns inputs | |
| return 0, err | |
| } |
| outPtr, outLen, err := c.writeBytes(outputs) | ||
| if err != nil { | ||
| dests.discard(c) // export never called: wrapper still owns everything | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
The inputs buffer leaks on this error path. It was allocated by writeBytes(inputs) and is only freed by the guest during the export; since the export is never invoked here, the wrapper still owns it and must free it alongside dests.discard(c). Repeated failures with large inputs will steadily grow the WASM heap.
Suggestion:
| outPtr, outLen, err := c.writeBytes(outputs) | |
| if err != nil { | |
| dests.discard(c) // export never called: wrapper still owns everything | |
| return 0, err | |
| } | |
| outPtr, outLen, err := c.writeBytes(outputs) | |
| if err != nil { | |
| c.freeWASM(inPtr, inLen) | |
| dests.discard(c) // export never called: wrapper still owns everything | |
| return 0, err | |
| } |
- free wrapper-owned pre-export buffers on every pre-invocation error path: inputs in EstimateTransactionSize (2 paths), message/intent buffers in both intent functions (writeBytes inputs leak on array writer failure) - check Memory().Write result when writing table indices into the array buffer in both writers; on failure discard the whole array and error out instead of returning success with an uninitialized slot - fold keys/idxs/elems parallel slices into a single per-element record (externRefElement), removing positional coupling between the slices - writeBytes: free the malloc'd buffer if the memory write fails instead of returning 0 (caller could never free it) - distinguish pre-invocation call failures (unknown export name, arity mismatch) via errExportNotInvoked: externRefArray.finish discards the array when the export never ran, and releases otherwise (leak-safe on traps, where the guest may have partially consumed the buffer) - drop duplicated externRefArray doc comment left above externRefElement
d3ca841 to
d730860
Compare
1 participant
Summary
EstimateTransactionSize(and the two intent exports) corrupted the WASM module's dlmalloc heap on every call. The deferredfreeStringArray/freeUint8ArrayArrayin the Go ABI wrapper operated on memory the guest had already freed, and the next malloc-heavy export (EncodePartiallySignedTransaction,EncodeWitnessNoSignature, ...) trapped out-of-bounds. Consumers saw this as the conclude+create flow failing while conclude-only and create-only flows passed.This PR fixes the wrapper only. No consumer changes are required.
Root cause
The module's
Vec<String>/Vec<Vec<u8>>ABI (wasm-bindgen reference-types) transfers ownership of the argument array to the guest:__wbindgen_malloc(n*4, 4)→ writes externref table indices → calls the export.table.get→__wbindgen_string_get→ copy →__wbindgen_free), recycles the element slots via__externref_table_dealloc, and frees the array buffer itself — verified by inspecting the buffer bytes before/after (80 00slot index →9c 66dlmalloc freelist pointer).__externref_table_dealloc(garbage)(poisoning the guest slot free-list),refs.freeon wrong keys, and__wbindgen_freeon the already-freed buffer → double free → heap corruption.Affected call sites:
estimate_transaction_sizeEstimateTransactionSize(wasm/transactions.go)encode_signed_transaction_intentEncodeSignedTransactionIntent(wasm/intent.go)Invalid atoms amount: 10000000p)verify_transaction_intentVerifyTransactionIntent(wasm/intent.go)freeStringArraywith 0 dests was a no-op (ptr == 0), which is why 0-destination estimates never corrupted and all PST/conclude-only tests stayed green.Fix
Mirror what wasm-bindgen's own JS glue does for owned-Vec params — nothing to the guest-owned buffer:
externRefArraythat separates what the guest owns (the array buffer, after the export is invoked) from what the wrapper still owns (Gorefsregistry entries; plus the wrapper-allocated element backing buffers forVec<Vec<u8>>, which the guest only copies from and never frees).release(c)frees only those wrapper-owned resources, using records captured before the call — it never re-reads the (freed) array buffer or touches the externref table.discard(c)covers error paths where the export was never invoked and the wrapper still owns everything (also fixes a pre-existing leak inEstimateTransactionSizewhen the outputs write failed).freeStringArray/freeUint8ArrayArray/getWASMTableRefare deleted.Regression tests
wasm/ownership_regression_test.go(fails deterministically on the old code, passes now):TestEstimateTransactionSizeThenEncodeCanary— 3× estimate (fee-loop shape) with 2 valid destinations, canary (EncodeWitnessNoSignature+EncodeTransaction) after each. Pre-fix: OOB trap on the next malloc-heavy export.TestEncodeSignedTransactionIntentCanary—Vec<Vec<u8>>path, 3 signatures, then canary. Pre-fix: mangled heap reads.TestVerifyTransactionIntentCanary— secondVec<String>path, ownership-contract guard.Pre-fix red-phase output (before this PR's fix):
Testing
go test ./wasm/ -count=1— full suite greengo test ./wasm/ -count=2— repeat runs green (heap layout shake-out)go build ./... && go vet ./wasm/— cleanNotes for reviewers
EstimateTransactionSizeon an instance that later encodes, but with this fix the mixed flow is safe.