bun:ffi: fix double-free of base_name in linkSymbols error paths - #29974
bun:ffi: fix double-free of base_name in linkSymbols error paths#29974robobun wants to merge 1 commit into
Conversation
When a symbol fails to compile in linkSymbols(), the .failed cleanup branch manually freed value.base_name for every symbol in the map and then called function.deinit() on the failing one. Since function is one of symbols.values(), its base_name was freed twice. Replace the manual field frees in all three linkSymbols error branches with value.deinit(global), matching the adjacent compile() catch branch and the equivalent paths in dlopen. This also ensures previously compiled symbols' TCC state and step.failed.msg are released instead of leaked.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Review rate limit: 0/5 reviews remaining, refill in 7 minutes and 23 seconds. Comment |
|
Updated 12:48 AM PT - Apr 30th, 2026
❌ @robobun, your commit fcd97a2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29974That installs a local version of the PR into your bun-29974 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as duplicate of #29946, which contains the identical The one bit of extra coverage here is a test that triggers the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix replacing ad-hoc partial cleanup with Function.deinit(), with a regression test. The two inline notes are pre-existing issues (not introduced or worsened here) and can be follow-ups.
Extended reasoning...
Overview
This PR fixes a double-free in FFI.linkSymbols() (src/bun.js/api/ffi.zig) by replacing the manual free(base_name); arg_types.clearAndFree() pattern in three error branches (missing-ptr, .failed, .pending) with the canonical value.deinit(global) loop. The .failed branch previously freed base_name for every symbol in the loop and then called function.deinit() on the failing symbol, freeing its base_name a second time — a real ASAN-detected use-after-poison. The fix mirrors the adjacent compile() catch branch which already does this correctly. A subprocess-isolated regression test is added to test/js/bun/ffi/ffi-error-messages.test.ts.
Security risks
None. This is error-path resource cleanup in the FFI module; no new user input is parsed, no auth/crypto/permissions touched. The change strictly reduces memory-safety risk by eliminating a double-free and several leaks (TCC state, step.failed.msg) on error paths.
Level of scrutiny
Medium-low. FFI memory management is sensitive in principle, but this change is small and mechanical: three identical hunks each replacing a two-line manual-free body with a single value.deinit(global) call, plus reordering in the .failed branch so err.msg is read before its owner is freed. Function.deinit() already correctly handles base_name, arg_types, state, the compiled callback wrapper, and step.failed.msg, so delegating to it is strictly safer than the open-coded subset. The .failed branch no longer calls function.deinit() separately because function is one of symbols.values() and is covered by the loop.
Other factors
- The PR description includes a clear repro, ASAN trace, root-cause analysis, and verification that the new test fails without the fix and passes with it.
- Both inline bug-hunter comments are explicitly tagged [Pre-existing]: (1) two analogous leak patterns remain in
FFI.open()(dlopen), and (2)ZigString.init(err.msg).toErrorInstance()may create a non-copying string whose backing buffer is freed by the subsequentdeinit— but the old code had the identicaltoErrorInstance→function.deinit()sequence, so this PR neither introduces nor worsens it. These are reasonable follow-ups but do not block this fix. - No CODEOWNERS entry covers this path; no outstanding human review comments.
| for (symbols.values()) |*value| { | ||
| allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); | ||
| value.arg_types.clearAndFree(allocator); | ||
| value.deinit(global); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing, but worth noting since the PR description cites dlopen's cleanup as the model: FFI.open() still has the same leak class in two error paths. The dylib.lookup() orelse branch (~ffi.zig:1110-1118) uses the manual free(base_name); arg_types.clearAndFree() pattern instead of value.deinit(global), leaking the TCC.State of any previously-compiled symbol; and the .failed branch (~ffi.zig:1138-1146) registers the deinit loop with defer but then calls symbols.clearAndFree() inline, so the deferred loop runs over an already-empty map and frees nothing. Both are one-line fixes identical in spirit to the linkSymbols changes here and would be a natural addition to this PR.
Extended reasoning...
What the bug is
This PR fixes the error-cleanup paths in FFI.linkSymbols() by replacing the manual free(base_name); arg_types.clearAndFree() pattern with value.deinit(global), and the PR description explicitly cites "the equivalent cleanup in dlopen" as the model. However, FFI.open() (the dlopen implementation) still contains two error paths with the same leak class that this PR fixes:
-
The
dylib.lookup() orelsebranch (around src/bun.js/api/ffi.zig:1110-1118) still uses the manual-free pattern:const resolved_symbol = dylib.lookup(*anyopaque, function_name) orelse { const ret = global.toInvalidArguments("Symbol \"{s}\" not found in \"{s}\"", ...); for (symbols.values()) |*value| { bun.default_allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); value.arg_types.clearAndFree(bun.default_allocator); } symbols.clearAndFree(bun.default_allocator); dylib.close(); return ret; };
This frees
base_nameandarg_typesbut never callsFunction.deinit(), so it never freesstate(theTCC.State) orstep.failed.msg. -
The
.failedbranch (around src/bun.js/api/ffi.zig:1138-1146) does calldeinit, but viadefer:.failed => |err| { defer for (symbols.values()) |*other_function| { other_function.deinit(global); }; const res = ZigString.init(err.msg).toErrorInstance(global); symbols.clearAndFree(bun.default_allocator); dylib.close(); return res; },
In Zig, the
deferbody runs at scope exit — aftersymbols.clearAndFree()has already executed inline. By the time the deferred loop runs,symbols.values()returns an empty slice, so the loop iterates zero times and nothing is freed.
Code path that triggers it
The open() loop processes each symbol completely (lookup → compile() → switch on step) before moving to the next iteration. Function.compile() allocates a TCC.State and stores it in function.state on success.
Step-by-step proof for case (1) — dlopen(lib, { good: {...}, missing: {...} }) where good exists in the library and missing does not:
- Iteration 1:
dylib.lookup("good")succeeds →good.compile()runs → allocates aTCC.Stateintogood.state→good.step = .compiled. - Iteration 2:
dylib.lookup("missing")returnsnull→ enters theorelsebranch. - The cleanup loop runs over both symbols and frees only
base_name+arg_typesfor each. good.state(a liveTCC.State) is never freed → leaked.
Step-by-step proof for case (2) — dlopen(lib, { good: {...}, bad: {...} }) where both resolve in the library but TinyCC compilation of bad fails (e.g. handleTCCError sets bad.step = .failed):
- Iteration 1:
goodis looked up and compiled →good.stateholds aTCC.State. - Iteration 2:
badis looked up,compile()runs, TinyCC reports an error →bad.step = .failedwith an allocatederr.msg. - The
.failedarm registersdefer for (symbols.values()) |*f| f.deinit(global);. resis built fromerr.msg.symbols.clearAndFree()runs inline — the map's backing arrays are freed andvalues()is now empty.return res;triggers thedefer.symbols.values()returns a zero-length slice, so thedeinitloop body never executes.- Net result:
good.base_name,good.arg_types,good.state(TCC.State),bad.base_name,bad.arg_types, andbad.step.failed.msgall leak.
Why existing code doesn't prevent it
For (1), the adjacent compile() catch, .failed, and .pending branches in open() all already call value.deinit(global) — the dylib.lookup() orelse branch is simply the one that was missed when the others were converted. For (2), the code appears correct at a glance because it does call deinit on every value, but Zig's defer semantics mean the loop's iteration source (symbols.values()) is evaluated at scope-exit time, after clearAndFree() has emptied it. This is exactly the ordering bug this PR avoids in the linkSymbols .failed branch by running the deinit loop inline (after building res, before clearAndFree).
Impact
Each leaked TCC.State is a full TinyCC compiler state (relocated code, symbol tables, etc.) — typically tens of KB or more per symbol. In long-running processes that probe for symbols or repeatedly attempt dlopen against libraries with partial symbol coverage, this accumulates. The .failed path additionally leaks every base_name, every arg_types buffer, and the duplicated TinyCC error message string. These are not crashes or correctness bugs in user-visible behavior — they are pure memory leaks on the error path — but they are the same class of bug this PR is explicitly fixing in linkSymbols.
How to fix
Both are one-line changes mirroring this PR's linkSymbols edits:
dylib.lookup() orelsebranch: replace the loop body withvalue.deinit(global);(identical to the change at lines 1225-1227 of this PR)..failedbranch: drop thedeferand run thedeinitloop inline after buildingresbut beforesymbols.clearAndFree():This is exactly the structure this PR uses for the.failed => |err| { const res = ZigString.init(err.msg).toErrorInstance(global); for (symbols.values()) |*other_function| { other_function.deinit(global); } symbols.clearAndFree(bun.default_allocator); dylib.close(); return res; },
linkSymbols.failedbranch at lines 1244-1252.
These are pre-existing — the PR doesn't touch FFI.open() — but since the PR description holds dlopen up as the reference implementation, it's worth flagging that the reference has the same bug.
| // Build the error before deinit: `err.msg` is owned by `function.step` | ||
| // and `function` is one of `symbols.values()`. | ||
| const res = ZigString.init(err.msg).toErrorInstance(global); | ||
| for (symbols.values()) |*value| { | ||
| allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); | ||
| value.arg_types.clearAndFree(allocator); | ||
| value.deinit(global); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing, but worth noting since this PR's new comment implies the ordering makes it safe: ZigString.init(err.msg).toErrorInstance() does not copy the bytes — Zig::getErrorInstance uses toString() (unlike its siblings getTypeErrorInstance/getSyntaxErrorInstance/getRangeErrorInstance which use toStringCopy()), so for an untagged Latin1 ZigString it hits WTF::StringImpl::createWithoutCopying and the returned Error's .message directly references err.msg's heap buffer. After the value.deinit(global) loop frees step.failed.msg, reading e.message from JS is a use-after-free. The same pattern exists in dlopen's .failed branch and FFI.callback's .failed branch; the simplest local fix is ZigString.fromUTF8(err.msg).toErrorInstance(global) (or change getErrorInstance in helpers.h to use toStringCopy).
Extended reasoning...
What the bug is
Building the JS Error before the deinit loop is necessary but not sufficient, because ZigString.init(...).toErrorInstance() does not copy the message bytes for plain Latin1 strings. The returned Error object's .message property is a JSString whose StringImpl points directly into err.msg's heap allocation. The subsequent value.deinit(global) loop frees that allocation (via Function.deinit → val.allocator.free(val.step.failed.msg) when .allocated == true), so any later read of e.message from JavaScript dereferences freed memory.
Code path
-
err.msgis heap-allocated. TinyCC reports the compilation error throughhandleTCCError(ffi.zig:1531), which setsthis.step = .{ .failed = .{ .msg = this.allocator.dupe(u8, msg), .allocated = true } }. -
ZigString.init(err.msg)produces an untagged pointer.ZigString.init(ZigString.zig:481-483) just stores.{ ._unsafe_ptr_do_not_use = slice.ptr, .len = slice.len }— no UTF-8 tag, no UTF-16 tag, no external/global tag. -
toErrorInstance→Zig::getErrorInstance→toString()takes the no-copy path. Insrc/bun.js/bindings/helpers.h,getErrorInstance(line ~383) callstoString(*str). For an untagged, non-UTF8, non-external Latin1ZigString,toString()falls through to its final branch (helpers.h:115-118):return WTF::String(WTF::StringImpl::createWithoutCopying({ untag(str.ptr), str.len }));
Note the contrast:
getTypeErrorInstance,getSyntaxErrorInstance, andgetRangeErrorInstance(helpers.h:395-413) all calltoStringCopy(*str).getErrorInstanceis the lone outlier that uses non-copyingtoString(). -
JSC::createErrorstores the sameStringImpl.createErrordoesputDirect(vm.propertyNames->message, jsString(vm, message));jsStringjust refs the existingStringImplfor length > 1, no byte copy. -
The deinit loop frees the buffer. Back in
linkSymbols, the loop reaches the failing function andFunction.deinitexecutesif (val.step == .failed and val.step.failed.allocated) val.allocator.free(val.step.failed.msg). TheJSStringbacking the returned Error's.messagenow points into a freed mimalloc allocation. -
JS reads dangling memory. When user code does
String(e.message),console.log(e), or the error propagates to top-level, the freed buffer is dereferenced.
Why ordering alone doesn't help
The PR's comment ("Build the error before deinit: err.msg is owned by function.step") correctly addresses construction-time safety — you can't pass a freed pointer to ZigString.init. But it does not address post-return lifetime: the constructed JSString still aliases the buffer with no ownership, so freeing after construction is just as bad as freeing before. The comment encodes a stronger invariant than actually holds.
Step-by-step proof
Take the PR's own new test case:
linkSymbols({ "not a valid C identifier!": { ptr: cb.ptr, args: [], returns: "void" } });- TinyCC fails to compile the generated C (the symbol name is emitted verbatim), and
handleTCCErrorheap-allocates the diagnostic intostep.failed.msgwith.allocated = true. linkSymbolsenters the.failedbranch, buildsres = ZigString.init(err.msg).toErrorInstance(global)—res.message'sStringImplnow points at the duped diagnostic buffer.- The deinit loop frees that buffer.
- The test only checks
threw = trueand never touchese.message, so ASAN doesn't flag it. Change the catch tocatch (e) { threw = true; String(e.message); }and run under ASAN to observe the heap-use-after-free.
Status and scope
This is pre-existing: the old code did exactly the same thing (const res = ZigString.init(err.msg).toErrorInstance(global); function.deinit(global);). The PR neither introduces nor worsens it. The identical pattern also exists in:
dlopen's.failedbranch (const res = ZigString.init(err.msg).toErrorInstance(global)followed bydefer for ... other_function.deinit(global))FFI.callback's.failedbranch (const message = ZigString.init(err.msg).toErrorInstance(globalThis); func.deinit(globalThis);)Bun__FFI__cc's.failedbranch
Flagging it here because the PR rewrites these exact lines for memory-safety reasons and the new comment reads as though the lifetime issue is handled.
Fix
Either of:
- Call site:
const res = ZigString.fromUTF8(err.msg).toErrorInstance(global);—fromUTF8sets the UTF-8 tag, sotoString()takes thefromUTF8ReplacingInvalidSequencespath which allocates a copy. - Root cause: change
getErrorInstanceinsrc/bun.js/bindings/helpers.hto usetoStringCopy(*str)likegetTypeErrorInstance/getSyntaxErrorInstance/getRangeErrorInstancealready do. This fixes all four call sites at once.
1 participant
Repro
The symbol name is emitted verbatim into the generated C source, so TinyCC fails to compile it and
linkSymbolsenters the.failedcleanup branch.Under a debug/ASAN build this aborts with:
Cause
In the
.failedbranch the cleanup loop does:functionis one ofsymbols.values(), so itsbase_nameis freed in the loop and then freed again insideFunction.deinit. The.pendingand missing-ptrbranches use the same manual-free pattern and additionally leak the TCCstate(andstep.failed.msg) of any symbols that already compiled successfully before the failure.Fix
Call
value.deinit(global)for every symbol in all three error branches, matching the adjacentcompile() catchbranch and the equivalent cleanup indlopen. In the.failedbranch, construct the error instance before the loop sinceerr.msgis owned byfunction.step.Verification
bun bd test test/js/bun/ffi/ffi-error-messages.test.ts— 6 passsrc/stashed, the new test aborts under ASAN with the trace above; with the fix it passes.