ffi: reject non-void return types on threadsafe JSCallbacks - #32782
ffi: reject non-void return types on threadsafe JSCallbacks#32782robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThe PR updates ChangesFFI callback contract and validation
TLS peer certificate assertion
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated by robobun
Status: diff is green across all test lanes; the only CI red is one Mac agent's artifact downloadBuild #65562 on 7e37720 finished with 284 jobs passed, zero on agent All My one retrigger (9e3f439, dropped in the rebase) is spent; the diff is complete and I'm not pushing further. Three commits, 7 files:
All automated review findings are addressed (0 unresolved threads). |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither of those is fixed by this PR, so I'm not adding the
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches a CODEOWNER-owned path (packages/bun-types/) and changes Zig::getErrorInstance in helpers.h — a helper with ~150 call sites across the tree — so it's worth a human look.
Extended reasoning...
Overview
This PR fixes a SEGV/garbage-return when a threadsafe JSCallback is given a non-void return type. It spans seven files across four layers: the Rust FFI runtime (ffi_body.rs, host_fns.rs) moves the dead guard into Function::compile_callback and deletes the unreachable copies; the JS glue (src/js/bun/ffi.ts) makes JSCallback's constructor throw returned native errors like dlopen/cc/linkSymbols already do; the C++ bindings (src/jsc/bindings/helpers.h) switch getErrorInstance from toString (no-copy) to toStringCopy to fix an ASAN-confirmed UAF; and docs/types/tests are updated to match.
Security risks
None identified. FFI is inherently memory-unsafe by design, but this change strictly tightens validation (rejecting a configuration that previously read uninitialized memory) and fixes a heap-use-after-free in error-message construction. No new attack surface is introduced.
Level of scrutiny
Moderate-to-high. The FFI and JS changes are narrow and well-justified. However, getErrorInstance in helpers.h backs ZigString::to_error_instance, which has ~150 call sites across ~48 files (sockets, S3, fetch, DNS, shell, server, crypto, etc.). The author audited callers and the change matches the three sibling helpers (getTypeErrorInstance/getSyntaxErrorInstance/getRangeErrorInstance), so it looks correct — but a one-line change with that blast radius merits a human sanity check, particularly around any callers that might have relied on the external-string adoption path (the author says none do).
Other factors
packages/bun-types/ffi.d.tsis covered by CODEOWNERS (*.d.ts//packages/bun-types/), so a codeowner sign-off is required regardless.JSCallbacknow throws on invalid options where it previously yieldedptr: undefined; this is a (correct) behavior change that could surface in user code that was silently misconfigured.- Good test coverage is added (positive and negative cases), and the PR description reports the existing FFI test matrices pass unchanged. The bug-hunting system found no issues.
|
The bot is partly right. #31779 (from @EffortlessSteven, opened June 3) independently found and fixed two of the three bugs here, three weeks before this PR. My duplicate search was scoped to robobun-authored PRs, so I opened this without seeing it. That is on me, and credit for the dead-guard find belongs there; I have added #31779 to the description. The two PRs are not identical, so I am leaving both open for a maintainer to pick rather than closing either unilaterally. The concrete differences: 1. Where the guard lives. #31779 fixes the check in place, in dlopen("libc.so.6", { strlen: { args: ["cstring"], returns: "u64", threadsafe: true } });
// today: works. under #31779: throws "Threadsafe functions must return void"This PR puts the guard in 2. How far the error-surfacing reaches. #31779 throws the 3. Where the tests live. #31779's tests sit inside 4. Not in #31779. The Either outcome is fine by me:
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/tls/node-tls-connect.test.ts`:
- Around line 313-314: The TLS certificate test is still pinned to bun.sh’s live
certificate fields, which makes it fragile when AIA contents change. Update the
assertions in the node-tls-connect test to avoid depending on specific live
`infoAccess` entries from `translatePeerCertificate`; use a fixture/local
certificate instead, or assert only the parser output shape and general presence
of parsed fields rather than `CA Issuers - URI` from the live cert.
🪄 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: 1641b32a-9624-4d40-b8c4-88a617837c8e
📒 Files selected for processing (8)
docs/runtime/ffi.mdxpackages/bun-types/ffi.d.tssrc/js/bun/ffi.tssrc/jsc/bindings/helpers.hsrc/runtime/ffi/ffi_body.rssrc/runtime/ffi/host_fns.rstest/js/bun/ffi/ffi-error-messages.test.tstest/js/node/tls/node-tls-connect.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/ffi/host_fns.rs
There was a problem hiding this comment.
Thanks for fixing the docs nit. I'm deferring to a human here: the getErrorInstance → toStringCopy change in helpers.h reaches every to_error_instance caller (the audit looks careful, but it's broad), and you've explicitly left the #31779 overlap / Co-authored-by credit as a maintainer call.
Extended reasoning...
Overview
The PR fixes a threadsafe JSCallback segfault by (1) moving the "Threadsafe functions must return void" guard from the shared symbol parser into Function::compile_callback (and deleting the dead checks in both generate_symbol_for_function copies), (2) making the JSCallback constructor throw returned-Error results instead of destructuring them into ptr: undefined, and (3) switching Zig::getErrorInstance from toString (createWithoutCopying) to toStringCopy to close a heap-use-after-free hit under ASAN. It also updates docs/types, adds 11 JSCallback validation tests, and carries an unrelated CI-unblock commit dropping the OCSP - URI assertion from the live bun.sh TLS test.
Security risks
None introduced. The changes are defensive (rejecting an unsound configuration earlier, copying error-message bytes that were previously borrowed). The helpers.h change closes latent UAFs rather than opening any. No auth/crypto/permissions surface.
Level of scrutiny
Medium-high. The FFI guard relocation and the ffi.ts error-throw are small and well-tested. But getErrorInstance → toStringCopy is a one-line change with wide blast radius: per the description it affects JSGlobalObject::create_error's interpolated slow path and every ZigString::to_error_instance caller in the tree. The author's audit (no caller relies on non-copying/external-adoption, six more latent UAFs closed) reads correct and matches the three sibling get*ErrorInstance helpers that already use toStringCopy, but a maintainer should sanity-check that claim since it touches a core JSC binding header.
Other factors
- The author explicitly flagged the overlap with #31779 (and #30165, #31892) as a maintainer decision — which PR lands, where the guard lives, and Co-authored-by credit. That alone warrants human review rather than bot approval.
- An unrelated TLS test change is bundled in to unblock CI; the author offered to split it out.
- My prior inline nit (
CString(ptr, length)→CString(ptr, 0, length)in the docs example) was addressed in a051989; both call sites indocs/runtime/ffi.mdxare now correct. - The bug-hunting system found no issues on the current revision; tests added are ungated and exercise both the new guard and the previously-swallowed validation errors.
A threadsafe JSCallback's trampoline binds FFI_Callback_threadsafe_call,
which is void in C++: it posts a task to the JS thread and returns before
the JS function runs, so there is no return-value channel. The generated
TCC stub declares the bound symbol as returning an EncodedJSValue, so any
non-void return type reads a stale register. Integer return types produce
silent garbage, and pointer-shaped ones (u64, ptr, cstring) dereference
the garbage as a JSCell and SEGV.
Bun already had a "Threadsafe functions must return void" validation, but
it read function.threadsafe (the out-param, still at its default false)
instead of the local `threadsafe` parsed a few lines earlier, so it never
fired. Move the guard into Function::compile_callback, the layer that
owns the trampoline, instead of restoring it in
generate_symbol_for_function: that parser also serves dlopen, cc,
CFunction, and linkSymbols, where threadsafe is documented as
JSCallback-only and is a parsed-but-unread no-op, and firing it there
would newly reject valid native-function specs. Delete the dead check
from both copies of the shared parser.
Two more bugs had to be fixed for the guard to be visible:
- JSCallback's constructor destructured { ctx, ptr } straight out of
nativeCallback's return value. Validation errors are returned, not
thrown, from the native side, so every one of them was silently
swallowed into ptr: undefined. dlopen, cc, and linkSymbols all already
do `if (Error.isError(result)) throw result`; do the same here.
- Zig::getErrorInstance in helpers.h used toString(*str), which builds
the Error's message with StringImpl::createWithoutCopying: the JS
string borrows the caller's bytes for the lifetime of the Error. Its
three siblings (getTypeErrorInstance, getSyntaxErrorInstance,
getRangeErrorInstance) all use toStringCopy. The new guard's message
is a Box<[u8]> inside Step::Failed, owned by a local Function that
drops when callback() returns, so reading the thrown error's .message
is a heap-use-after-free under ASAN. Switch getErrorInstance to
toStringCopy. An audit of every ZigString::to_error_instance caller
found none relying on the non-copying or external-adoption path, and
found six more latent UAFs this closes: JSGlobalObject::create_error's
interpolated slow path, two sites in AsyncModule, and the other three
FFI Step::Failed arms.
Also fix the docs/runtime/ffi.mdx example, which paired threadsafe: true
with returns: "bool" (a configuration that has never worked), document
the constraint on FFIFunction.threadsafe in bun-types, and declare
[Symbol.dispose] on the JSCallback type (the runtime already implements
it).
The CString constructor signature is (ptr, byteOffset?, byteLength?), so `new CString(ptr, length)` treats `length` as a byte offset and reads a NUL-terminated string starting at `ptr + length`, past the buffer the native caller handed over. The Strings section in the same document already shows the correct call, `new CString(ptr, 0, byteLength)`. Fix both JSCallback examples, the pre-existing non-threadsafe one and the thread-safe one this PR rewrote, to `new CString(ptr, 0, length)`.
9e3f439 to
238f2ff
Compare
Bun__createFFICallbackFunction heap-allocates an FFICallbackFunctionWrapper holding two JSC::Strong roots (the user's callback closure and the global object). compile_callback only stores that pointer into Step::Compiled.ffi_callback_function_wrapper on the success path, and Function::Drop only calls FFICallbackFunctionWrapper_destroy when step is Compiled. Every earlier exit (the TCCMissing return and the five self.fail() arms for TinyCC compile/relocate/symbol failures) leaked the wrapper and permanently rooted the user's closure. Wrap the wrapper in a scopeguard at the allocation site, mirroring the existing scopeguard for the TinyCC state a few lines below, and disarm it with ScopeGuard::into_inner when ownership transfers into Step::Compiled. Pre-existing; the new threadsafe/non-void guard added in the parent commit is placed before this allocation, so it did not add a new leak instance. The failure paths in question require TinyCC to fail on Bun-generated trampoline source, which is an internal-error condition not deterministically reachable from a test; the success path (into_inner) is exercised by the existing 72-test JSCallback matrix under ASAN.
There was a problem hiding this comment.
Both prior review findings are addressed and this pass found nothing new. Not auto-approving: the getErrorInstance → toStringCopy change in helpers.h is a cross-cutting fix to a shared helper (audited per the description, but worth a human glance), and the #31779 overlap / credit question you already flagged is a maintainer call.
Extended reasoning...
Overview
Seven files: the threadsafe/non-void guard moves from the (dead) parser check in generate_symbol_for_function (both ffi_body.rs and host_fns.rs copies) into Function::compile_callback; JSCallback's JS constructor now throws returned Errors like dlopen/cc/linkSymbols; Zig::getErrorInstance switches toString → toStringCopy to match its three siblings and close a UAF; compile_callback wraps ffi_wrapper in a scopeguard (added in 7e37720 in response to my last comment); plus docs, bun-types, and new validation tests in ffi-error-messages.test.ts.
Security risks
None identified. bun:ffi is inherently an unsafe-by-design module; the changes here strictly reduce unsafety (reject a config that read uninitialized memory, copy error-message bytes instead of borrowing freed ones, deref a leaked GC root on failure). No new attacker-reachable surface.
Level of scrutiny
Medium-high. The Rust/TS/test/docs changes are localized and well-covered by the new tests, but two pieces warrant human eyes:
helpers.h: a one-word change togetErrorInstance, but that helper backsZigString::to_error_instanceandcreate_error_instance(format_args!(...))across the whole codebase. The PR description says every caller was audited and none depends on the non-copying / external-adoption semantics; that audit is plausible (the three siblingget*ErrorInstancehelpers already usetoStringCopy) but is exactly the kind of cross-cutting claim a maintainer should sanity-check.compile_callbackscopeguard (7e37720): correct by inspection —ScopeGuard::into_innerdisarms beforeStep::Compiledtakes ownership, and every earlier exit derefs the wrapper — and ASAN-verified per the author, but it's new resource-lifetime code in a function with five failure exits.
Other factors
- The author explicitly left the #31779 overlap (where the guard lives, which PR lands,
Co-authored-bycredit) as a maintainer decision; that alone makes this not auto-approvable. - Both of my earlier inline findings (the
CString(ptr, length)docs bug and the pre-existingffi_wrapperleak) were fixed in a051989 and 7e37720 respectively; all review threads are resolved. - CodeRabbit's TLS-test concern was discussed and deferred to a follow-up; that file is no longer in the changed-files set for this PR.
- Test coverage for the new behavior is good (19 assertions, including the previously-swallowed
returns: buffer/napi_env/ unknown-type errors).
|
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.) |
|
Thanks. Checked each piece against
Two small leftovers that are not in any open PR I can see:
Happy to send a tiny PR for those two if useful. |
2 participants
Reproduction
On
mainthis SEGVs (panic(main thread): Segmentation fault at address 0x4F048506), with the top frame inJSC__JSValue__toUInt64NoTruncatedecoding garbage bits as aJSCell*. With an integer return type instead (returns: "int"), there is no crash, only a silently wrong result: a callback that returns7hands the native caller a stale value like1325696257.Cause
A threadsafe
JSCallbackbindsFFI_Callback_threadsafe_callas the TCC trampoline'sFFI_Callback_callsymbol. That function isvoidin C++: it posts a task to the JS thread and returns before the JS function runs, so there is no return-value channel. But the generated C stub declaresFFI_Callback_callas returning anEncodedJSValue, so for any non-void return type it reads an uninitialized return register.Bun already has a guard for exactly this, with the message
Threadsafe functions must return void, but it never fires. Ingenerate_symbol_for_functionit testsfunction.threadsafe(the out-param, still at itsDefaultoffalse) instead of the localthreadsafeparsed a few lines earlier; the field is only assigned below the check.Even with a working guard,
JSCallback's constructor swallows the error. Native validation errors are returned (not thrown), and the constructor destructures{ ctx, ptr }straight out ofnativeCallback's result, so anErrorreturn becomesptr: undefined.dlopen,cc, andlinkSymbolsall doif (Error.isError(result)) throw result;JSCallbackis the only entry point that does not.Fix
Reject the configuration in
Function::compile_callback(src/runtime/ffi/ffi_body.rs), the layer that owns the trampoline, via the existingself.fail()/Step::Failedpath, before any allocation. The dead checks in both copies ofgenerate_symbol_for_function(ffi_body.rs,host_fns.rs) are deleted rather than fixed in place: that parser also servesdlopen,cc,CFunction, andlinkSymbols, wherethreadsafeis documented as JSCallback-only and is a parsed-but-unread no-op (self.threadsafehas exactly one reader:callback()passing it tocompile_callback). Firing the check there would newly reject valid native-function specs.Throw native validation errors from the
JSCallbackconstructor (src/js/bun/ffi.ts), matchingdlopen/cc/linkSymbols. This also unswallows the other construction errors (returns: "buffer",returns: "napi_env", unknown types), which previously succeeded withptr: undefined.Copy the error message in
Zig::getErrorInstance(src/jsc/bindings/helpers.h). Running the new test underbun bd(ASAN) tripped a pre-existing heap-use-after-free:getErrorInstancebuilds theError's message withtoString(*str), which usesWTF::StringImpl::createWithoutCopying: the JS string borrows the caller's bytes for the lifetime of theError. Its three siblings (getTypeErrorInstance,getSyntaxErrorInstance,getRangeErrorInstance) all usetoStringCopy(*str). The new guard's message is aBox<[u8]>insideStep::Failed, owned by a localFunctionthat drops whencallback()returns, so reading.messageon the thrown error reads freed memory.Switching
getErrorInstancetotoStringCopymatches its siblings. I audited everyZigString::to_error_instancecaller in the tree: none relies on the non-copying or external-adoption behavior (so nothing leaks), and six more latent UAFs are closed by the same line:JSGlobalObject::create_error's interpolated slow path (everycreate_error_instance(format_args!("...{}..."))in the codebase formats into a localVec<u8>; the// it alwayas clonescomment there was the author's assumption, and this change makes it true), two sites inAsyncModule.rs, and the other three FFIStep::Failedarms. None of those had fired because the freed bytes usually survive intact under mimalloc, and the FFI ones additionally had the error swallowed by (2).Also fixed the
docs/runtime/ffi.mdxexample, which pairedthreadsafe: truewithreturns: "bool"(a configuration that has never worked), documented the constraint onFFIFunction.threadsafeinbun-types, and declared[Symbol.dispose]()on theJSCallbacktype (the runtime already implements it).Verification
test/js/bun/ffi/ffi-error-messages.test.ts:USE_SYSTEM_BUN=1 bun test(unfixed)toThrowassertions throw)bun bd test(fixed, ASAN)With only the Rust + JS fixes and (3) reverted,
bun bd testaborts with the ASAN report above.The existing
JSCallbackround-trip (48) andthreadsafe callback(24) matrices inffi.test.js, the rest oftest/js/bun/ffi/,test/js/web/html/FormData.test.ts(129), andtest/integration/bun-types/bun-types.test.tsall pass unchanged.Related
function.threadsaferead and the constructor error swallow three weeks before this PR was opened; my duplicate search only covered robobun-authored PRs and missed it. The two differ in where the guard lives (this PR scopes it tocompile_callback, sodlopen/cc/CFunctionspecs carrying the JSCallback-onlythreadsafeflag keep working), in whether theStep::Failederror path is also surfaced, and in test placement (ffi: reject non-void return types for threadsafe JSCallback #31779's tests sit inside theffi.test.jsblock gated on a fixture CI never builds). The full comparison is in this comment. If this PR is the one that lands, the guard fix should carry aCo-authored-byfor ffi: reject non-void return types for threadsafe JSCallback #31779's author.JSCallbackbug (JSBigIntallocated off the JS thread for 64-bit integer arguments) and, as a secondary change, the same dead guard; it predates the Rust rewrite and is currently conflicting. Landing the guard here lets that PR drop the overlapping commit on rebase.helpers.hget*ErrorInstancehelpers (createError->createErrorAllowEmptyMessage); orthogonal, small textual conflict for whichever lands second.