bun:ffi: throw when calling a symbol after close() instead of jumping into freed JIT memory - #29946
bun:ffi: throw when calling a symbol after close() instead of jumping into freed JIT memory#29946robobun wants to merge 10 commits into
Conversation
|
Updated 11:05 PM PT - May 4th, 2026
❌ @robobun, your commit 039a89e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29946That installs a local version of the PR into your bun-29946 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFFI runtime functions are rooted at creation; teardown now detaches generated JS trampolines via a new native hook ( Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/ffi/ffi.test.js`:
- Around line 978-1034: The tests currently call lib.close() directly and may
exit before it runs on assertion failures; wrap the usage of each linked/dlopen
library (the lib created in the "linkSymbols: throws...", "linkSymbols:
zero-arg...", and "dlopen: throws..." tests) in a try/finally and call
lib.close() in the finally block (similar to how cb.close() is handled) so the
native library is always closed even on early failures; locate the lib variable
in these tests and add try/finally around the assertions, calling lib.close() in
the finally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5413e2e4-874d-4414-a537-cd25add860b4
📒 Files selected for processing (1)
test/js/bun/ffi/ffi.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/api/ffi.zig`:
- Around line 813-815: When cc() aborts mid-loop the CompileC defer path
(CompileC.deinit() / SymbolsMap.deinit()) skips per-symbol teardown so
JSFFIFunction roots and their TCC.State leak; modify the abort/unwind path to
call each Function's teardown: locate the symbols map used by cc() and in
CompileC.deinit() (or the cc() defer that runs on error) iterate the stored
JSFFIFunctions and invoke the Function deinit/close routine (e.g.,
Function.deinit() or the JSFFIFunction-specific detach/unprotect method) so that
cb.protect()ed roots are unprotected and each per-symbol TCC.State is freed on
error. Ensure this cleanup runs before SymbolsMap.deinit() completes to avoid
leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20540e52-31d1-4686-8bb5-3c7243ef659a
📒 Files selected for processing (2)
src/bun.js/api/ffi.zigtest/js/bun/ffi/ffi.test.js
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bun.js/api/ffi.zig:1170-1172— The newcb.protect()is balanced byunprotect()only insideFunction.deinit(), but several mid-loop error paths inopen()(dlopen) andlinkSymbols()never calldeinit()on earlier-compiled symbols, so thoseJSFFIFunctions become permanent GC roots. Affected: dlopen's symbol-not-found path (1115-1121), linkSymbols' missing-ptrpath (1231-1235),.failed(1252-1259) and.pending(1263-1267); thecc()path at line 815 has the same issue (already flagged by CodeRabbit). The TCC.State leak on these paths is pre-existing, but the GC-root leak is new in this PR — the simplest fix is to make each cleanup loop callvalue.deinit(global)(mirroring thecompile() catchbranches that already do).Extended reasoning...
What the bug is
This PR adds
cb.protect()immediately after eachJSFFIFunctionis created in the per-symbol compile loops ofcc()(line 815),open()/dlopen (line 1172) andlinkSymbols()(line 1284). The matchingunprotect()lives only insideFunction.deinit()(gated onffi_callback_function_wrapper == null). On the happy path that's fine —FFI.close()iteratesthis.functionsand callsdeinit()on every value.But each of these loops also has mid-loop error-return paths that perform partial cleanup which never reaches
Function.deinit()for the entries that already succeeded. On those paths, everycb.protect()taken on earlier iterations is never balanced, permanently rooting thoseJSFFIFunctioncells in the GC.The specific code paths
open()(dlopen), ffi.zig:1108-1176 — thedylib.lookup()failure branch at 1113-1121 loopssymbols.values()and only doesfree(base_name)+arg_types.clearAndFree(). It never callsvalue.deinit(). Contrast with the three other error paths in the same function (compile()catch at 1133-1135,.failedat 1142-1144,.pendingat 1152-1154), which do callvalue.deinit(global)on every entry and are therefore fine — only the symbol-not-found path leaks.linkSymbols(), ffi.zig:1226-1289 — three branches leak:- missing-
ptrat 1229-1237: cleanup loop only freesbase_name+arg_types; .failedat 1251-1260: same partial frees, thenfunction.deinit()only on the current (failed, never-protected) symbol;.pendingat 1262-1268: same partial frees.
Only the
compile()catch path (1244-1246) correctly callsvalue.deinit(global)for all values.cc(), ffi.zig:781-818 — already flagged by CodeRabbit at line 815: oncompile()throw /.failed/.pending, the only cleanup is thedefer { if (hasException()) compile_c.deinit(); }at 623-627, andCompileC.deinit()→SymbolsMap.deinit()(546-551) iterates onlymap.keys()to free key strings — it never walksFunctionvalues, soFunction.deinit()never runs.Why existing code doesn't prevent it
The partial-cleanup loops predate this PR; they were written when a
Functionwhose step is.compiledheld nothing that needed releasing beyondbase_name/arg_types/the TCC state. Now that.compiledalso implies a livegcProtect(), the only routine that releases it isFunction.deinit(), and these branches simply don't call it. The asymmetry with the sibling branches that do calldeinit()(e.g. dlopen'scompile()catch) shows the omission is accidental rather than by design.Step-by-step proof
StringArrayHashMappreserves insertion order, so:// linkSymbols, missing-ptr branch linkSymbols({ a: { returns: "i32", ptr: somePtr }, // iteration 0 b: { returns: "i32" }, // iteration 1 — no ptr });
- Iteration 0:
ahas a ptr → skips the missing-ptr check →compile()succeeds →.compiled→cb.protect()runs at line 1284. - Iteration 1:
b.symbol_from_dynamic_library == null→ enters 1229-1237. The cleanup loop freesbase_name/arg_typesfor both entries, thensymbols.clearAndFree()andreturn ret. a'sFunction.deinit()is never reached, soa'sunprotect()never runs. TheJSFFIFunctionforais now a permanent GC root for the lifetime of the VM.
The dlopen analogue is equally easy:
dlopen(libc, { strlen: {...}, nonexistent_xyz: {...} })—strlencompiles and is protected, thennonexistent_xyzfailsdylib.lookup()and hits 1115-1121.Impact
Each occurrence permanently pins one
JSFFIFunctioncell per already-compiled symbol. The objects are small, but the leak is unbounded if the user retries (e.g. probing for optional symbols in a loop), and it's reachable from very ordinary public-API misuse like a typo'd symbol name. The per-symbolTCC.Stateleak on the same paths is pre-existing — these branches never freedval.stateeither — but the GC-root leak is new in this PR because theprotect()calls are new.How to fix
Make each affected cleanup loop call
value.deinit(global)instead of (or in addition to) the manualfree(base_name)/arg_types.clearAndFree(), mirroring what thecompile()catch branches at 1133-1135 / 1244-1246 already do —Function.deinit()already handlesbase_name,arg_types, the newunprotect(), andstate.deinit()in one place. Forcc(), either iteratecompile_c.symbols.map.values()callingdeinit()in the errordefer, or haveSymbolsMap.deinit()walk values as well as keys.🔬 also observed by coderabbitai
- missing-
There was a problem hiding this comment.
All four issues I flagged earlier are now addressed and the current revision looks correct to me, but given this touches FFI/JIT memory lifetime + GC rooting across several error-unwind paths and changes the non-Windows hot-path dispatch (every FFI call now goes through the trampoline indirection), I'd like a human to give the final state a once-over before merge.
Extended reasoning...
Overview
This PR makes calling a bun:ffi symbol after lib.close() throw a TypeError instead of jumping into freed TinyCC JIT pages. It touches four files: src/bun.js/api/ffi.zig (per-symbol protect()/unprotect() rooting, Function.deinit teardown reordering, several mid-loop error-path cleanup rewrites, new Bun__FFIFunction_setClosed extern), src/bun.js/bindings/JSFFIFunction.{cpp,h} (cross-platform trampoline with null check, new setFunction() setter, createForFFI now always routes through the trampoline instead of baking the TinyCC pointer into the NativeExecutable), and test/js/bun/ffi/ffi.test.js (six new regression tests).
Security risks
The change is strictly safety-improving on the headline path (SEGV → catchable TypeError). The new protect()/unprotect() pairing and the dynamicDowncast in Bun__FFIFunction_setClosed introduced their own UAF/leak hazards during review, but all four were fixed (JSCallback-path gating, GC rooting of compiled.js_function, and two rounds of mid-loop error-unwind balancing including the defer-after-clearAndFree() ordering bug). I don't see remaining security issues in the final diff.
Level of scrutiny
High. This is native memory-safety code at the FFI/JSC-GC boundary: it adds GC roots that must be balanced across ~7 error-unwind branches in open()/linkSymbols()/cc(), reorders state.deinit() relative to js_function detachment, and changes how every FFI call dispatches on non-Windows (one extra load + indirect call per invocation, matching what Windows already paid). The fact that it took four bug-hunter iterations to converge — each finding a real correctness issue — is itself a signal that the invariants here are subtle.
Other factors
Test coverage for the new behavior is good (both FFIBuilder-wrapped and raw JSFFIFunction paths, idempotent close(), delete-symbol-then-GC-then-close, mid-loop failure, JSCallback-borrowed-symbol). All earlier review threads (mine and CodeRabbit's) are resolved. What I'd want a human to sanity-check: (1) the perf tradeoff of always-trampoline on non-Windows is acceptable, (2) the final set of error-unwind branches in open()/linkSymbols() is now exhaustively balanced, and (3) the interaction with #29858's GC-reachability changes is as described.
… into freed JIT memory JSFFIFunction::createForFFI previously baked the TinyCC-compiled trampoline address directly into a NativeExecutable on non-Windows platforms. When lib.close() ran tcc_delete and freed the JIT pages, the JSFFIFunction (which is still reachable via the cached symbols object or any user-held reference) still pointed its executable at the freed code, so calling it jumped into unmapped/reused memory. Route all createForFFI() calls through the static trampoline (the same path Windows already used) so the native pointer lives in the mutable m_function field. On close(), Function.deinit now nulls m_function via Bun__FFIFunction_setClosed before freeing the TCC state; the trampoline checks for null and throws a TypeError instead of dereferencing freed memory.
Function.deinit is shared between the dlopen/linkSymbols/cc paths (where step.compiled.js_function is the owned JSFFIFunction whose trampoline lives in the TCC state being freed) and the JSCallback path (where it is the user-supplied callback, which may itself be a JSFFIFunction from a still-open library). Only detach on the former, identified by ffi_callback_function_wrapper == null.
compiled.js_function is stored in the Zig heap and was only reachable from GC via the mutable symbols object. If the user deleted a symbol from lib.symbols and a GC ran before lib.close(), Function.deinit would pass a stale encoded JSValue to Bun__FFIFunction_setClosed, which dereferences it via dynamicDowncast. Protect each JSFFIFunction when storing it in compiled.js_function (dlopen/linkSymbols/cc paths only; the JSCallback path already roots the user callback via FFICallbackFunctionWrapper's Strong<JSFunction>) and unprotect it in Function.deinit after detaching.
SymbolsMap.deinit (reached via CompileC.deinit when cc() throws after some symbols have already been compiled) previously only freed the map keys. Each Function value's TCC.State, arg_types, and now the protect()ed JSFFIFunction root were leaked. Since each map key aliases the Function's base_name allocation, route the per-key cleanup through Function.deinit instead, which frees base_name plus the rest. Split out Function.deinitWithoutGlobal so SymbolsMap can call it without a JSGlobalObject (the parameter was already unused).
dlopen's symbol-not-found branch and linkSymbols' missing-ptr / .failed / .pending branches did partial cleanup (free base_name + arg_types) instead of calling Function.deinit on each value, so any symbol that had already reached .compiled on an earlier iteration leaked its protect()ed JSFFIFunction root and TCC.State. The sibling compile() catch branches already used value.deinit(); make these consistent. Also fixes a pre-existing double-free in linkSymbols' .failed branch where base_name was freed in the partial-cleanup loop and then again via function.deinit() on the current (failed) symbol.
…fter The deferred for-loop evaluated after symbols.clearAndFree() had already emptied the map, so it iterated nothing and no Function.deinit ever ran on this path (pre-existing; now also leaks the protect() root). Match the .pending branch and the fixed linkSymbols .failed: capture err.msg first, then run the deinit loop inline before clearAndFree().
3627839 to
4e34f76
Compare
|
Superseded by #31960. This PR implemented the fix against src/bun.js/api/ffi.zig, but the Rust port (#30412) moved the implementation to src/runtime/ffi/ffi_body.rs and the Zig file is now a non-compiled porting reference, so these changes can no longer take effect. #31960 applies the same design (trampoline on all platforms, invalidate on close) to the current implementation, plus coverage for the CFunction finalization registry and the finalize() leak path. |
1 participant
Problem
dlopen()/cc()/linkSymbols()create aJSFFIFunctionfor each symbol whose native entry point is TinyCC-JIT-compiled trampoline code living in a per-functionTCC.State. On non-Windows,JSFFIFunction::createForFFIwrapped that trampoline address directly as the function'sNativeExecutable.lib.close()runstcc_deleteon every function's state, freeing/unmapping the JIT pages — but theJSFFIFunctionobjects are still reachable from JS (via the cachedsymbolsobject or any reference the user captured before closing), and their executables still point at the freed code. Calling one jumps straight into freed memory.Repro
Fix
JSFFIFunction::createForFFInow always routes through the statictrampolinehost function (previously Windows-only), so the TinyCC pointer lives in the mutablem_functionfield rather than being baked into aNativeExecutable.trampolinechecksm_functionfor null and throwsTypeError: Cannot call an FFI function after the library has been closedinstead of dereferencing it.Function.deinit(called fromFFI.close()) nullsm_functionvia a newBun__FFIFunction_setClosedbefore freeing the TCC state.The indirection is one load + one indirect call per FFI invocation — the same cost Windows was already paying — and makes the freed pointer unreachable from the JS call path.
This is independent of #29858, which keeps the FFI wrapper alive while any symbol is GC-reachable; that PR does not guard user-initiated
close().Verification
New tests in
test/js/bun/ffi/ffi.test.jscoverlinkSymbols(with and without args so both theFFIBuilder-wrapped and rawJSFFIFunctionpaths are exercised) anddlopen:inc(1)afterlib.close()TypeError: Cannot call an FFI function after the library has been closedAll existing
test/js/bun/ffi/*tests pass.