bun:ffi: throw when calling a symbol after close() instead of jumping into freed TinyCC memory - #31960
bun:ffi: throw when calling a symbol after close() instead of jumping into freed TinyCC memory#31960robobun wants to merge 8 commits into
Conversation
close() frees the TinyCC-compiled wrapper code while the JSFFIFunction objects remain reachable and callable from JS. Calling one jumped into freed executable memory. Route all platforms through the trampoline (Windows already did for ABI reasons) and null-check the function pointer there. Each Rust Function now holds a Strong reference to its JS function and invalidates it in Drop before the TCC state is destroyed. The CFunction finalization registry now keys collection on the native function instead of the wrapper.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Updated 8:54 AM PT - Jul 7th, 2026
❌ @Jarred-Sumner, your commit 945c292 has some failures in 🧪 To try this PR locally: bunx bun-pr 31960That installs a local version of the PR into your bun-31960 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughPrevents calling FFI symbols after a library is closed by adding a JSC trampoline guard and invalidate bridge, rooting and invalidating JSFFIFunctions in Rust with reordered teardown, updating FinalizationRegistry tracking, and adding tests and docs for close behavior. ChangesFFI Symbol Use-After-Free Prevention
Possibly Related Issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/bun/ffi.ts`:
- Around line 563-567: The FinalizationRegistry heldValue currently uses
result.symbols[identifier].close (a bound function) which retains result and
prevents GC; instead register a lightweight token object (e.g. const token = {})
as the held value when calling
cFunctionRegistry.register(result.symbols[identifier].native, token), and store
a Map from token to a WeakRef of result plus the identifier (e.g.
cFunctionCloseMap.set(token, { libRef: new WeakRef(result), id: identifier }));
update onCloseCFunction to accept the token, lookup the WeakRef from
cFunctionCloseMap, call libRef.deref()?.close(id) if present, then delete the
map entry—this removes the strong reference cycle while still allowing cleanup
to call result.close for result.close.bind(result) indirectly.
In `@src/jsc/bindings/JSFFIFunction.cpp`:
- Around line 114-120: The invalidate path currently only calls
Zig::JSFFIFunction::invalidate which poisons the trampoline target but leaves
the public exported pointer reachable; update Bun__JSFFIFunction__invalidate to
also clear/neutralize the exported ptr so re-wrapping oldSymbol.ptr cannot jump
into freed code. Specifically, in Bun__JSFFIFunction__invalidate add logic to
zero out the exported symbol (the field that holds the TinyCC wrapper / public
.ptr — e.g. m_exported or the public ptr member on Zig::JSFFIFunction) or call a
new helper on Zig::JSFFIFunction that clears both m_function and the exported
ptr; ensure this change covers the code paths where
generate_symbol_for_function, linkSymbols, and CFunction may receive numeric
ptrs so the public ptr is set to 0 when invalidate runs.
🪄 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: 0c4e6532-db1e-4ff0-b485-7fdc8cc6297b
📒 Files selected for processing (6)
docs/runtime/ffi.mdxsrc/js/bun/ffi.tssrc/jsc/bindings/JSFFIFunction.cppsrc/jsc/bindings/JSFFIFunction.hsrc/runtime/ffi/ffi_body.rstest/js/bun/ffi/ffi.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A library left open at process exit leaks its TCC states by design, which LeakSanitizer reports on the ASAN CI lane.
JSCallback, linkSymbols and CFunction all require TinyCC, which is disabled on Windows ARM64. Matches the gate used by cc.test.ts and ffi-error-messages.test.ts.
There was a problem hiding this comment.
No further issues from me — both earlier comments are addressed (Windows ARM64 gate landed in e3cd54d). Deferring to a human for the final look since this reworks native FFI lifecycle across Rust/C++/JSC (Strong roots in Function, Drop/teardown ordering in FFI::close/finalize, and routing all POSIX FFI calls through the trampoline).
Extended reasoning...
Overview
This PR fixes a production use-after-free where calling a bun:ffi symbol after close() jumps into freed TinyCC-JIT'd memory. It touches six files across three languages:
JSFFIFunction.{h,cpp}— addsinvalidate()and an unconditionaltrampoline(previously Windows-only) that null-checksm_functionand throwsTypeError; adds theBun__JSFFIFunction__invalidateC ABI entry point.ffi_body.rs— adds anOption<jsc::Strong>toFunctionso each compiled symbol roots itsJSFFIFunction;Function::dropinvalidates before freeing the per-function TCC state;FFI::close()reorders teardown (functions → shared TCC state → dylib);FFI::finalize()releases just the Strong roots when leaking; refactors three call sites intoattach_compiled_symbol().ffi.ts—CFunction's FinalizationRegistry now registers.nativeinstead of the wrapper.ffi.test.js— six new tests in alibrary close()describe (subprocess crash regression, wrapper/.native invalidation, library independence, dlopen close, CFunction close, GC retention), plus a drive-by fix to usesuffixinstead of hardcoded.dylib. Gated withdescribe.skipIf(isWindows && isArm64)after my earlier comment.ffi.mdx— one sentence documenting the new throw behaviour.
Security risks
This is memory-safety hardening, not new attack surface. bun:ffi already grants arbitrary native-pointer access; this change strictly removes a UAF crash class by turning it into a catchable TypeError. The CodeRabbit note about .ptr re-wrapping is correctly out of scope (raw pointers are inherently unverifiable). No auth/crypto/permissions touched.
Level of scrutiny
High. This is exactly the kind of change that benefits from a maintainer's eyes:
- Cross-language GC lifecycle —
jsc::Strongheld in a Rust struct whoseDropcalls back into C++ to mutate a JSC cell, with the comment noting "Functionis only created and dropped on the JS thread, whichStrongrequires." That invariant and thefinalize()path (releasing roots while intentionally leaking TCC states) are subtle. - Teardown ordering in
FFI::close()is now load-bearing (functions must drop before the sharedcc()state and dylib). - Hot-path change on POSIX — every FFI call now goes through
trampolineinstead of the directNativeExecutablebaked pointer. The PR description quantifies this as one load + one branch + one indirect call, matching what Windows already paid, but it's a deliberate perf trade-off a maintainer should ack.
Other factors
- The bug-hunting system found nothing on the latest revision.
- Both of my earlier inline comments are resolved: the stderr-assertion nit was reasonably declined (ASAN lane noise), and the Windows ARM64 TinyCC gate was applied in e3cd54d.
- Both CodeRabbit threads are resolved with sound author rebuttals (pre-existing FinalizationRegistry cycle is acknowledged and out of scope;
.ptrrevocation is impossible by design). - Test coverage is thorough, including a subprocess fixture that reproduces the original segfault on an unfixed build.
- Remaining CI failures on the latest reported commit (musl LTO link errors,
bunx.test.ts) are unrelated infrastructure/flakes, not FFI.
The PR is well-reasoned and well-tested; I'm deferring solely because native memory-management changes of this shape should get a human sign-off, not because of any open concern.
|
CI status: the diff is green. ffi.test.js passes on every lane, including Linux ASAN x64/aarch64 (the LeakSanitizer failure was fixed in aeb9051) and Windows ARM64 (TinyCC gate in e3cd54d). The only remaining red is test/cli/install/bunx.test.ts ("should handle package that requires node 24", which runs bunx --bun @angular/cli@latest --help against the live npm registry and got exit 3). It has failed on a different lane in each of the last four builds (macOS, Windows x64, Windows x64-baseline, Linux ASAN) while passing on the rest, and does not touch anything in this PR. Ready for review. |
|
Reproduced with the Branch is current with main as of 945c292 (maintainer merge). The diff vs main is unchanged (6 files, 215+/51-). The Build 69816: the two red darwin-26 aarch64 jobs both died in "buildkite-agent artifact download timed out after 120s" before running any tests (artifact-storage hiccup, same class seen on 66367 and 61680). The remaining lanes are still draining. On every completed run, ffi.test.js passes on all lanes including Linux ASAN and Windows ARM64. Retrying those two jobs should turn the build green. PR: #31960 |
There was a problem hiding this comment.
No issues found, but this is a cross-language memory-lifecycle change (Rust Drop → C++ invalidate, new jsc::Strong rooting, unconditional trampoline on the POSIX hot path) that warrants a human sign-off.
Extended reasoning...
Overview
This PR fixes the most frequent non-OOM native crash in production by making FFI symbols throw a TypeError after close() instead of jumping into freed TinyCC executable memory. It spans three layers: JSFFIFunction.{h,cpp} adds an unconditional trampoline with a null-check and a new Bun__JSFFIFunction__invalidate C entrypoint; ffi_body.rs adds a jsc::Strong root per Function, an attach_compiled_symbol helper used by bun_ffi_cc/open/link_symbols, invalidation in Function::drop, reordered teardown in FFI::close(), and Strong-release in the intentional-leak path of FFI::finalize(); ffi.ts retargets CFunction's FinalizationRegistry to .native. Tests cover subprocess crash regression, wrapper+.native invalidation, library independence, real dlopen, CFunction.close(), and a GC-stress retention case. Docs add one sentence.
Security risks
The change is strictly a hardening: it converts a use-after-free into a catchable exception. It does not add new attack surface, expose data, or touch auth/permissions. The CodeRabbit note about .ptr re-wrapping is correctly out of scope (raw pointers in bun:ffi are inherently caller-trusted, and a copied integer cannot be revoked).
Level of scrutiny
High. This is native memory-lifecycle code at the JSC/Rust/TinyCC boundary: it introduces a new GC root (jsc::Strong) whose lifetime is tied to Rust Drop, reorders destruction in close()/finalize(), and changes the POSIX FFI call path from a direct jump to a trampoline+indirect call on every invocation. Each of these is correct by my reading and well-justified in comments, but the interactions (Drop ordering, Strong release in the leak path, finalize vs close, JS-thread-only Strong) are subtle enough that a maintainer familiar with the FFI/JSC ownership model should confirm — particularly the per-call overhead trade-off on POSIX and the finalize() Strong-release semantics.
Other factors
All prior review threads (CodeRabbit ×2, my Windows ARM64 gate and stderr nit) are resolved; the ARM64 gate was applied in e3cd54d and the author reports CI green on all lanes after the merge with main. The bug-hunting system found nothing. The diff is well-commented, the test coverage is thorough, and the design rationale in the description is detailed. This is a good PR — it just isn't in the "simple/mechanical" bucket that auto-approval targets.
…se-uaf # Conflicts: # test/js/bun/ffi/ffi.test.js
|
Merged main again (bab9156). Two small conflicts, both in the test file:
The native side auto-merged cleanly on top of #32792 (TerminationException handling) and #32787 (NaN purification). Re-verified with a fresh debug build: ffi.test.js is 12 pass, 0 fail. |
There was a problem hiding this comment.
No issues found, but this is a native memory-lifecycle change spanning C++/Rust/JS (new jsc::Strong roots, Drop-ordered invalidation, and switching POSIX FFI calls to the trampoline path), so it warrants a human pass on the design.
Extended reasoning...
Overview
This PR fixes a use-after-free where calling a bun:ffi symbol after close() jumps into freed TinyCC-JIT'd memory. It touches src/jsc/bindings/JSFFIFunction.{cpp,h} (unconditional trampoline with null-check, new invalidate() + Bun__JSFFIFunction__invalidate C ABI), src/runtime/ffi/ffi_body.rs (each Function now holds a jsc::Strong to its JSFFIFunction; Drop invalidates before freeing TCC state; FFI::close() teardown reordered; FFI::finalize() releases roots on the intentional-leak path; three call sites refactored into attach_compiled_symbol), src/js/bun/ffi.ts (FinalizationRegistry target changed from wrapper to .native), plus tests and a one-line docs note.
Security risks
None in the traditional sense — this removes an exploitable-class UAF (jump into freed executable memory) and replaces it with a catchable TypeError. The remaining raw-pointer surface (linkSymbols accepting numeric ptrs) is pre-existing and by design, as already discussed and resolved in the CodeRabbit thread.
Level of scrutiny
High. This is native memory-lifecycle code coordinating GC roots (jsc::Strong), Rust Drop ordering, and TinyCC executable-page lifetime across three languages. The Strong must be correctly released on every path (close(), finalize() leak path, mid-loop error paths) or it leaks JS objects; the invalidation must run strictly before tcc_delete on every path or the original UAF remains. It also changes the POSIX FFI hot path from a direct NativeExecutable call to an indirect trampoline call — a deliberate perf trade-off the author quantifies, but one a maintainer should sign off on.
Other factors
The bug-hunting system found nothing. All prior review threads (CodeRabbit ×2, my Windows-ARM64 gating finding, my stderr nit) are resolved. Test coverage is solid: subprocess crash regression, wrapper/.native invalidation, library independence, real dlopen, CFunction.close(), and a GC-stress retention test; the author reports the suite fails as expected on an unfixed ASAN build. CI is green per the author's status updates (only an unrelated flaky bunx.test.ts remains). The design looks correct to me, but the cross-language lifecycle and hot-path change put it outside what I'll approve without a human reviewer.
|
For whoever does the human pass, the three lifecycle points in the review above are each enforced in one place:
On CI: the only red job on the current build is the darwin-aarch64 lane failing in "buildkite-agent artifact download timed out after 120s" before any tests ran; the test lanes that completed are green. |
|
The close()-after-use UAF fix here looks real and well-tested on its own terms, but this should not carry
The likelier #31941 root cause is what #32013 addresses (TinyCC JIT run memory allocated from the CRT heap, then Suggest editing the body to remove the |
|
Agreed, and updated. I had over-indexed on the trampoline fingerprint; the three points hold: no close() in the repro, one failure after ~70k good calls rather than the deterministic first-call fault this PR produces, and the shared 0x...108 low bits read as a fixed field offset in a data deref. The body now says "Related to #31941 by crash fingerprint ... does not explain that report" and points at #32013 as the likely root cause for that issue, so merging this will not auto-close it. The hedge line is gone. |
|
Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @robobun for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
|
There is one piece #35246 did not pick up: calling a Repro on a library that genuinely unmaps on dlclose (libc-family libraries mask it because other references keep them resident): // cc -shared -fPIC -o libt42.so t42.c
int fortytwo(void) { return 42; }const { dlopen } = require("bun:ffi");
const lib = dlopen("/tmp/libt42.so", { fortytwo: { args: [], returns: "i32" } });
lib.symbols.fortytwo(); // 42
lib.close(); // dlclose unmaps the library
lib.symbols.fortytwo(); // SEGVDebug/ASAN build of main: Where the gap lives now: Two smaller notes:
Happy to take another run at this against the new architecture if you want it Bun-side; say the word. |
2 participants
Problem
Calling a
bun:ffisymbol after its library has been closed jumps into freed TinyCC-JIT'd memory. This is currently the most frequent non-OOM native crash in the wild (~155 events/day across1.3.x), fingerprinting on Windows as:On POSIX the same use-after-free jumps straight from JSC into TCC memory with no Bun frame, so those events land in anonymous noise buckets.
Cause
Each
dlopen()/cc()/linkSymbols()symbol owns aTCC::State;tcc_relocateallocates the compiled wrapper inside it.FFI::close()(src/runtime/ffi/ffi_body.rs) destroys those states, but nothing invalidates theJSFFIFunctionobjects, which stay reachable (cachedsymbolsobject, or any reference captured before closing) and callable:JSFFIFunction::createForFFIbaked the TCC pointer directly into theNativeExecutable, so JSC calls freed memory with no check anywhere.JSFFIFunction::trampoline(SYSV vs MS x64 ABI), which didfunction->function()(...)with zero validation.CFunctionadditionally registered its JS wrapper in a FinalizationRegistry whose cleanup callsclose(), while exposing the inner native function as.native, so collection of the wrapper could in principle free code that is still callable.Fix
JSFFIFunction::createForFFInow always routes through the statictrampoline(previously Windows-only), keeping the TCC pointer in the mutablem_functionfield. The trampoline throwsTypeError: Cannot call this FFI function: its library has been closedwhenm_functionis null. Cost on POSIX is one load, one predictable branch and an indirect call per FFI invocation, the same dispatch Windows has always used. Only TinyCC-compiled functions are affected; internal host functions created withadd_ptr_property = falsekeep the direct path.Functionholds ajsc::Strongto theJSFFIFunctionit created, andFunction::dropinvalidates it (via the newBun__JSFFIFunction__invalidate) before destroying the TCC state. This coversclose()and every mid-loop error path, and works even thoughsrc/js/bun/ffi.tsreplacessymbolsentries with plain JS wrappers (walking the symbols object would miss the real functions).FFI::close()clears the functions map first, so every JS function is invalidated before the sharedcc()state and the dylib are freed.FFI::finalize()(unclosed library GC'd while symbols are still reachable) now releases just theStrongroots: the leaked TCC states keep the code valid, and the JS functions remain collectable exactly as before.CFunction's FinalizationRegistry registers.nativeinstead of the wrapper. Note this registry currently never fires at all: the held close callback references the library, which references the registered target, and a held value that reaches its target prevents collection. That makes it a pre-existing leak rather than a crash source; making it actually fire is out of scope here.Verification
New tests in
test/js/bun/ffi/ffi.test.js(library close()describe): a subprocess fixture running the exact crash scenario, wrapper +.nativeinvalidation, independence of other open libraries, a realdlopen(libc)close-then-call (gated on the existinglibPathprobe),CFunction.close(), and a GC stress test proving aCFunctionstays callable while only.nativeis retained (10xBun.gc(true)).On the unfixed debug+ASAN build the suite fails exactly as production does: the subprocess exits with code 132 dying inside freed TCC memory, and the in-process variant segfaults the runner at a wild heap address (
Segmentation fault at address 0x5B7DBA0CA04). With the fix, all pass, including underBUN_JSC_validateExceptionChecks=1.Also ran: full
ffi.test.js,cc.test.ts,ffi-error-messages.test.ts,addr32.test.ts,ffi-viewSource-non-object.test.ts,test/regression/issue/30717.test.ts,test/napi/napi-value-ffi.test.ts.Notes:
cc()entry point shares the identical teardown (sameFunction::dropinvalidation); it has no separate test here because TinyCC's setjmp error handling conflicts with ASan (see the existing skips incc.test.ts).FFI runnerfixture check uses the platformsuffix, but thedlopencall hardcoded/tmp/bun-ffi-test.dylib, so the suite could never load its fixture anywhere but macOS. With the fixture actually built on Linux debug+ASAN, that pre-existing suite shows unrelated failures (exhaustive integer-identity loops time out, and an ASan bad-free inJSBuffer__bufferFromPointerAndLengthAndDeinitviatoBuffer) that exist on main and are not touched by this PR. CI does not build that fixture, so nothing changes there.Supersedes #29946, which implemented the same design against the Zig implementation before the Rust port (#30412) orphaned it.
Related to #31941 by crash fingerprint (
JSFFIFunction::trampolineis the last Bun frame for every Windows FFI call, so that bucket groups all FFI-adjacent crashes), but this PR does not explain that report: the repro there never callsclose(), and the shared low-bit fault addresses point to a fixed-offset data dereference rather than a jump into freed code. See #32013 for the likely #31941 root cause. This PR is worth landing on its own: every path that frees TinyCC executable memory now invalidates the JS functions first, so calling a torn-down symbol surfaces as a catchableTypeErrorinstead of a segfault.