Repository navigation
[JSC] bun:ffi: a threadsafe JSFFICallback's entry thunk stays callable after its cell and VM are gone - #437
Conversation
…e after its cell and VM are gone Native code may call a `threadsafe` callback's entrypoint from any thread at any time -- including after the global object that created it has been destroyed (a terminated worker), which the embedder cannot always order against close(). Everything that entrypoint touched lived in the GC cell: the JIT'd thunk (freed by ~JSFFICallback), the signature, the closed/pending word. A call arriving after the worker's VM teardown (or after GC of a closed callback) executed freed thunk memory or read a dead cell; in Bun that is a null Ref<Signature> assert on debug and a segfault on release. The threadsafe thunk now targets a ThreadSafeRefCounted FFI::ThreadsafeCallbackHandle instead of the cell. The handle owns the thunk code, a ref to the signature, the closed-bit/pending-count word and the embedder's routing token; the cell and each ThreadsafeInvocation hold refs to it, and only the owning thread reads handle->callback(). When a threadsafe callback's cell is destroyed it clears that pointer and sets the closed bit; if the callback was never close()d -- native code was never told to stop calling -- the handle is additionally kept alive for the rest of the process, so late calls count themselves out and return instead of jumping into freed code. close()d callbacks are released as before. runThreadsafeInvocation() returns early for a handle whose cell is gone, and retireThreadsafeInvocation() is exported for invocations an embedder accepted but will not run ($vm's drain uses it). Non-threadsafe callbacks are unchanged: they may only be called on the owning thread while it is inside the FFI call that handed them out.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits, I also checked: Signature is ThreadSafeRefCounted so the handle's Ref<Signature> is safe to drop from whichever thread releases the last invocation; the closed-bit/pending-count CAS logic is byte-identical to what previously lived in the cell; and retireThreadsafeInvocation() / runThreadsafeInvocation() both guard handle.callback() before touching the cell, so an invocation drained after cellDestroyed() doesn't dereference the dead cell.
Extended reasoning...
This is a non-trivial cross-thread lifetime restructuring in JIT-adjacent code, so I'm not auto-approving. The two inline findings are minor (an error-path leak and a header-hygiene nit); the note above records the deeper thread-safety concerns that were examined and found sound, so a human reviewer needn't re-derive them.
…orward-declare ThreadsafeCallbackHandle in FFICallbackThunk.h
…ot); root threadsafe callbacks VM-wide; retire under the API lock; testFFI second-VM regression test - ~JSFFICallback keeps the handle (and thunk) alive whenever an entrypoint was handed out, closed or not: there is no happens-before between a foreign thread leaving the thunk and this thread freeing it, and a close()-then-worker-exit-then-late-call reproduced the original crash. ~200-250 bytes per threadsafe callback ever created. - Threadsafe callbacks are Heap::protect()ed until closed and drained, so the cell cannot die (and sit unswept, still reachable through handle->callback() from a delivered invocation) while its VM lives on, as it could when rooted only through a collectable global object. - runThreadsafeInvocation retires (and possibly unroots) under the API lock; a call into a closed/dead handle is logged under verboseFFI. - testFFI: testThreadsafeCallbackOutlivesVM creates a threadsafe callback in a VM of its own, destroys that VM (unclosed / closed / with an invocation pending) and checks the entrypoint still drops calls and returns zero, and that the pending invocation retires harmlessly. Heap-use-after-free under ASAN with the previous close() policy.
Preview Builds
|
…ys the VM on the defensive path too
There was a problem hiding this comment.
I've reviewed this PR across four rounds and the current revision is clean — all five earlier findings (handle leak on the OOM path, missing forward-declare, retireThreadsafeInvocation lock requirement, testFFI defensive-path VM leak, missing <atomic> include) are addressed and no new issues were found. Given the scope — cross-thread lifetime management against GC/VM teardown, JIT thunk ownership, and a deliberate process-lifetime retention policy for executable memory — a human look is still warranted.
What was reviewed:
- The handle/cell/VM lifetime ordering:
cellDestroyed()runs before theRefPtrderef,heap.protectkeeps the cell live until closed+drained, and the intentionalref()in the destructor is now gated on an entrypoint existing. retireThreadsafeInvocationandrunThreadsafeInvocationboth reachunroot()under the API lock; the!callbackearly-return inrunThreadsafeInvocationcorrectly retires without touching the dead VM.- The atomic state word (
tryBeginInvocation/endInvocation/markClosedAndReportPending) is a straight move fromJSFFICallback— semantics unchanged. - The new
testThreadsafeCallbackOutlivesVMnow unconditionally destroys its per-iteration VM.
Extended reasoning...
Overview
This PR fixes a use-after-free where a threadsafe: true JSCallback's JIT'd entry thunk could be called by native code after the owning cell, global object, and VM had been destroyed (e.g. a terminated worker). The fix introduces FFI::ThreadsafeCallbackHandle, a ThreadSafeRefCounted heap object that owns everything the foreign-thread entry path touches (thunk code, signature, closed/pending state, embedder token). The thunk bakes in the handle pointer instead of the cell pointer; once an entrypoint is handed out the handle is kept alive for the process lifetime so late calls land in valid code and return zero. Threadsafe callbacks are additionally heap.protect()ed VM-wide until closed and drained. Nine files touched: the FFI runtime (FFIContext.h/cpp, JSFFICallback.h/cpp, FFICallbackThunk.h/cpp, BunFFI.h), the $vm test hook, and a new testFFI case.
Security risks
The change is in the direction of more safety — it eliminates a UAF on JIT'd executable memory. The new intentional-leak policy means each threadsafe callback that handed out an entrypoint retains ~200–250 bytes of executable + heap memory forever; this is documented and bounded by user callback creation, not attacker-controlled. No new untrusted-input parsing, no auth/crypto/permission surface. The atomic state machine is moved verbatim from the cell to the handle, so no new memory-ordering logic was introduced.
Level of scrutiny
High. This is JavaScriptCore runtime code with cross-thread lifetime reasoning against the GC and VM teardown, changes to JIT thunk generation, and a deliberate policy decision (never free a handed-out threadsafe thunk). The correctness argument depends on subtle ordering: ~JSFFICallback running during the VM's last sweep before anything else can observe handle.callback(), the heap.protect preventing the cell from dying while its VM lives, and the acq_rel on the state word ensuring foreign threads see the closed bit. These are the kinds of invariants a human familiar with JSC's heap and threading model should sign off on.
Other factors
- The author has been responsive: all five automated findings across three prior review rounds were fixed in follow-up commits (f6dfc99, d6c2d08, 963b501), and each fix is confirmed present in the current diff.
- The PR description documents ASAN verification against the original repro, Bun's FFI test suite (146 tests), and the new
testFFIcase covering unclosed / closed / pending-at-teardown VM destruction. - The only change since the last automated review is the one-line
#include <atomic>addition — trivially correct. - This run of the bug-hunting system found no issues.
I am not approving because the guidelines reserve automated approval for simple/mechanical changes, and this is neither; the design tradeoff (permanent per-callback executable-memory retention vs. a freelist follow-up the description mentions) is also worth a human acknowledging.
…tlives its VM) — to be replaced by the merge commit's autobuild
… callable after its cell and VM are gone (#437)" This reverts commit 8b6b046. A JSCallback's entrypoint is a C function pointer whose backing state belongs to a VM. Calling it after close() or after that VM has been torn down is use-after-free by the native caller, the same as any C callback, and should crash rather than silently return zero. #437 made that misuse "work" by leaking a handle, Signature and JIT thunk for every threadsafe callback ever created (out of the fixed executable pool), Heap::protect()ing every threadsafe callback, and dropping late calls on the floor. None of it is needed by a correct program. Document the contract on nativeEntrypoint() instead. No-Verification-Needed: straight revert of a merged commit plus a header comment
1 participant
Native code may call a
threadsafe: trueJSCallback's entrypoint from any thread at any time — including after the global object that created it has been destroyed (a terminated worker, orprocess.exit()inside one), which the embedder cannot always order againstclose(). Everything that entrypoint touched lived in the GC cell: the JIT'd thunk (released by~JSFFICallback), theSignature, the closed/pending word. A call arriving after the worker's VM teardown executed freed thunk memory or read a dead cell — in Bun a nullRef<Signature>assert on debug builds and a segfault on release:Change. The threadsafe thunk targets a
ThreadSafeRefCountedFFI::ThreadsafeCallbackHandleinstead of the cell. The handle owns the thunk code, a ref to the signature, the closed-bit/pending-count word (moved fromJSFFICallback) and the embedder's routing token; the cell and eachThreadsafeInvocationhold refs to it, and only the owning thread readshandle->callback(). Once an entrypoint has been handed out the handle — and with it the thunk — is never freed,close()d or not: there is no happens-before between a foreign thread leaving the thunk and the owning thread freeing it, so a call arriving afterclose(), or after the cell, global object and VM are gone, counts itself out and returns zero (logged underverboseFFI) rather than jumping into freed code. When the cell is destroyed it clears the handle's cell pointer and sets the closed bit; threadsafe callbacks are additionallyHeap::protect()ed until closed and drained, so a cell cannot die (and sit unswept, still reachable from a delivered invocation) while its VM lives on.runThreadsafeInvocation()returns early for a handle whose cell is gone and retires under the API lock, andretireThreadsafeInvocation()is exported for invocations an embedder accepted but will not run on the owning thread ($vm's drain uses it). Non-threadsafe callbacks are unchanged (callable only on the owning thread, inside the FFI call that handed them out).ThreadsafeInvocation::embedderContext()is preserved, so Bun's dispatch hook needs no change.Cost. One small heap object per threadsafe callback; the thunk and dispatch fast path are otherwise identical (the baked pointer is the handle instead of the cell). Every threadsafe callback that handed out an entrypoint keeps its handle + thunk (~200–250 bytes: a 64–96-byte thunk at 32-byte granularity, the handle, the executable-memory handle) for the rest of the process by design — that memory is what a late native call lands in. A per-signature freelist that re-arms retired thunks would bound this by the peak live count and is a possible follow-up.
Verified with a Bun debug+ASAN build against this branch: the repro above (both
terminate()and in-workerprocess.exit(), native thread calling for 1.5 s after the worker is gone) runs clean where it asserted 4/4 before, and calling a worker's threadsafe callback from the parent after the worker exited — closed before exit or not — returns 0; ordinary threadsafe delivery andclose()behave as before (closed callbacks are still collected, unclosed ones stay rooted); Bun'stest/js/bun/ffi/ffi.test.js(146 tests) andcc.test.tspass. NewtestFFIcasetestThreadsafeCallbackOutlivesVM(creates the callback in a VM of its own, destroys that VM unclosed / closed / with an invocation pending, then calls the entrypoint and retires the pending invocation): passes; with the previous "free on close()" policy its closed variant is a heap-use-after-free under ASAN. All 12testFFIcases pass (9552 checks).