bun:ffi: make cc() symbols' .ptr the address of the compiled C function - #38031
bun:ffi: make cc() symbols' .ptr the address of the compiled C function#38031robobun wants to merge 2 commits into
Conversation
Bun__CreateFFIFunctionValue stored the bits of the TinyCC-compiled JSC-ABI wrapper reinterpreted as a double, so symbols.fn.ptr was a denormal that CFunction and linkSymbols rejected and that C code received as NULL. Expose the native symbol instead, encoded with JSC::FFI::pointerToJSValue like the engine-native dlopen() symbols and JSCallback.
|
Warning Review limit reached
Next review available in: 24 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 9:00 AM PT - Aug 13th, 2026
✅ @robobun, your commit 0412c2d1a2f2f414aacabe177126fce32cd0b52b passed in 🧪 To try this PR locally: bunx bun-pr 38031That installs a local version of the PR into your bun-38031 --bun |
|
Status: fix is up for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that swaps the stale bit_cast<double> encoding for JSC::FFI::pointerToJSValue on the correct address, matching the dlopen/linkSymbols path.
What was reviewed:
- Confirmed
cc()is the onlyaddPtrField = truecaller; everyhost_fn::new_runtime_functioncall site passesfalseand hits the untouched branch, so its non-throwing signature is still sound. - Throw-scope shape (
RETURN_IF_EXCEPTIONafterpointerToJSValue,RELEASE_AND_RETURNon the tail) mirrorsBun__CreateJSCFFIFunction;call_zero_is_throw+?correctly propagates on the Rust side. - Test runs
cc()in a child process (matching the existing non-ASAN-skipped describes in this file) and covers the JS-wrappedcstringreturn path plus all three consumer shapes.
Extended reasoning...
Overview
Three files: src/jsc/bindings/JSFFIFunction.cpp (encode .ptr as symbolFromDynamicLibrary via JSC::FFI::pointerToJSValue under a throw scope, replacing jsNumber(std::bit_cast<double>(functionPointer))), src/runtime/ffi/ffi_body.rs (new_runtime_function now returns JsResult<JSValue> via jsc::call_zero_is_throw, single caller in the cc() compile loop propagates with ?), and a new test in test/js/bun/ffi/cc.test.ts.
Security risks
None material. The change swaps one pointer encoding for another that the rest of bun:ffi already uses; it does not expose any address that wasn't already reachable (the same symbolFromDynamicLibrary was already stored on the function object and dlopen()/linkSymbols() already surface the equivalent). No auth, crypto, or permission surface is touched.
Level of scrutiny
Moderate — JSC bindings and FFI are memory-adjacent, but the diff is small and mechanical. The new C++ path is a copy of the pattern in Bun__CreateJSCFFIFunction (throw scope, RETURN_IF_EXCEPTION, RELEASE_AND_RETURN). I grepped every caller of Bun__CreateFFIFunctionValue: the shared host_fn::new_runtime_function wrapper in src/jsc/host_fn.rs still returns bare JSValue, but all four of its call sites (hw_exports.rs ×2, node_os.rs, GeneratedBindings.rs) and the bindgen.ts codegen pass add_ptr_field = false, so they take the unchanged Bun__CreateFFIFunctionWithDataValue branch and can't observe the new throw. The local ffi_body.rs wrapper has exactly one caller, updated in this PR.
Other factors
The test follows the file's established child-process pattern (used by the "double <-> JSValue conversions" describe, which likewise runs cc() in a subprocess without an ASAN skip), drains stdout/stderr/exitCode concurrently, and asserts through all three consumer paths (CFunction, linkSymbols, "function" argument) plus the JS-wrapped cstring return case. The PR description states it fails on the current release and passes with debug+ASAN. No prior review comments to address.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// `cc()`'s variant of `host_fn::new_runtime_function`: with `add_ptr_property` | ||
| /// the C++ side encodes `symbol_from_dynamic_library` as the symbol's `.ptr`, | ||
| /// which allocates (and can throw) for addresses above 2^53. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
LGTM — a focused fix that swaps the leftover bit_cast<double> encoding for JSC::FFI::pointerToJSValue on the user's C symbol address, matching what dlopen()/linkSymbols() already do.
What was reviewed:
- Verified
cc()is the onlyaddPtrField=truecaller; every otherBun__CreateFFIFunctionValuecaller (host_fn::new_runtime_functionin hw_exports.rs, node_os.rs, GeneratedBindings.rs, bindgen.ts) passesfalseand takes the untouched branch. - Exception plumbing: throw scope +
RETURN_IF_EXCEPTION+RELEASE_AND_RETURNon the C++ side,call_zero_is_throw+?on the Rust side; the?sits alongside existingreturn Err(...)arms in the same match, so cleanup (the TCC scopeguard) is already handled. - The comment-cop bot flag on ffi_body.rs:169 was addressed by 0412c2d, which trimmed the 5-line doc comment to 3 lines; the remaining comment is a normal rustdoc explaining why the wrapper returns
JsResult. - The one CI failure (
test/bake/deinitialization.test.tson Windows x64) is in the dev-server suite and unrelated to bun:ffi.
Extended reasoning...
Overview
Three files: src/jsc/bindings/JSFFIFunction.cpp (the actual fix — encode .ptr via JSC::FFI::pointerToJSValue(symbolFromDynamicLibrary) instead of jsNumber(bit_cast<double>(functionPointer)), add a throw scope), src/runtime/ffi/ffi_body.rs (wrap the FFI call in call_zero_is_throw so the new possibly-throwing path returns JsResult, propagate with ? at the one call site, rename the parameter to match C++), and test/js/bun/ffi/cc.test.ts (a subprocess fixture asserting .ptr is a positive integer, distinct per symbol, and accepted by CFunction, linkSymbols, and as a "function" argument).
Security risks
None. bun:ffi is inherently a trust-the-user API; this change only fixes how an already-available native address is encoded as a JS number. No new capability is exposed — the address was already stored on function->symbolFromDynamicLibrary and reachable via other bun:ffi paths.
Level of scrutiny
Moderate: it touches C++ JSC bindings and unsafe Rust FFI, so I checked exception-scope hygiene and caller impact carefully. The change itself is ~10 lines of behavioral code plus wrapper plumbing. The old value was demonstrably useless (a denormal double), so there is no compatibility concern with changing it.
Other factors
- Confirmed every other route into
Bun__CreateFFIFunctionValuepassesaddPtrField=false:host_fn::new_runtime_functionat host_fn.rs:763 (used by hw_exports.rs, node_os.rs, GeneratedBindings.rs) and the bindgen.ts codegen all hard-codefalse, so they hit the unchangedBun__CreateFFIFunctionWithDataValuebranch and cannot see the new empty-return contract.host_fn::new_runtime_functionstaying-> JSValueis therefore fine. - The
?at ffi_body.rs:1257 sits inside a match whose other arms alreadyreturn Err(global_this.throw_value(...)); the enclosing scope has a_tcc_guardscopeguard, so early-return cleanup is already correct. RELEASE_AND_RETURNis used for the tail return, matching the repo's throw-scope conventions, and the author reportsBUN_JSC_validateExceptionChecks=1is clean.- The test follows the file's established subprocess pattern (matches the "double <-> JSValue conversions" describe block, which also runs
cc()in a child without an ASAN skip). It covers the cstring-wrapper path too, which copies.ptrin JS. - The comment-cop inline comment predates commit 0412c2d (the bot fired ~12s after that commit landed, i.e. on the prior push); the trimmed 3-line rustdoc is not a workaround justification.
- The lone CI failure is
test/bake/deinitialization.test.tssegfaulting on Windows 2019 x64 — bake dev-server code, no overlap with bun:ffi or any code this PR touches.
1 participant
Problem
ptrproperty on functions returned bycc()is a denormal double such as2.5541185406113e-311instead of an address.new CFunction({ ptr: symbols.f.ptr, ... })andlinkSymbols()throwTypeError: Symbol "CFunction" is missing a "ptr" field(the denormal truncates to 0), and passing it to a C function as a"function"/"ptr"argument calls NULL (Segmentation fault at address 0x0).Bun__CreateFFIFunctionValue(src/jsc/bindings/JSFFIFunction.cpp:81) storedjsNumber(std::bit_cast<double>(functionPointer)): the pointer's bits reinterpreted as a double, left over from an encoding bun:ffi stopped using. It was also the wrong pointer:functionPointeris the TinyCC-compiled wrapper with the JSC host-function ABI, not the user's C function.cc()is the only remaining caller that asks for theptrproperty (src/runtime/ffi/ffi_body.rs,new_runtime_function(..., true, ...)).dlopen()andlinkSymbols()moved to the engine-nativeJSC::JSFFIFunctionin bun:ffi: use the engine-native FFI when available #35246 and have a correct.ptr; bun:ffi: make lib.symbols.<fn>.ptr a numeric native address #34008 fixed this same line before bun:ffi: use the engine-native FFI when available #35246 and was closed as covered by it, which leftcc()out.Fix
Bun__CreateFFIFunctionValuenow setsptrtosymbolFromDynamicLibrary, the address of the user's C function thatcc()already passes in, encoded withJSC::FFI::pointerToJSValue.pointerToJSValueis the encoding every other bun:ffi pointer producer uses (dlopen()/linkSymbols()symbols,JSCallback.ptr) and every consumer decodes (CFunction,linkSymbols, pointer-typed arguments): a number below 2^53, an exact BigInt above it. The address exposed is the one with the declared C signature, which is the only one a consumer can call.pointerToJSValuecan throw when it has to allocate a BigInt, so the C++ side gets a throw scope and returns the empty value in that case; the Rust wrapper in ffi_body.rs goes throughcall_zero_is_throwandcc()propagates the exception. This is the same contract thedlopen()path has withBun__CreateJSCFFIFunction. Verified withBUN_JSC_validateExceptionChecks=1.Bun__CreateFFIFunctionValuepassaddPtrField = falseand take the untouched branch.test/js/bun/ffi/cc.test.ts, "symbols[name].ptr". It compiles four symbols and checks that.ptris a positive integer for a plain symbol and for areturns: "cstring"symbol (those are wrapped in JS and the wrapper copies.ptr), that the addresses differ, and that.ptrworks throughCFunction,linkSymbols, and as a"function"argument to anothercc()symbol. It runs in a child process because on the old code the last of those calls NULL. Fails on the current release (missing a "ptr" field), passes with this change.bun bd test test/js/bun/ffi/passes (debug + ASAN).Background
cc()compiles the user's C source with TinyCC, then for each requested symbol compiles a second small C function,JSFunctionCall(globalObject, callFrame), that unpacks the JS arguments and calls the user's symbol. The JS function object is created around that wrapper; the user's symbol address is looked up separately (tcc_get_symbol) and passed along assymbolFromDynamicLibrary.ptr(),read.ptr(),JSCallback.ptr,FFIType.ptrreturns). An earlier encoding reinterpreted the pointer bits as the double's bits;FFI.h(JSVALUE_TO_PTR/PTR_TO_JSVALUE) documents that change, and this property was the last producer still using the old form.JSC::FFI::pointerToJSValue(JavaScriptCoreFFIConversions.h) is the engine's encoder:nullfor 0, a number when the address fits in 53 bits, otherwise a BigInt (which is why it can throw: BigInts are heap allocated).call_zero_is_throwis the bun_jsc helper for calling a C++ function whose contract is "returns the empty JSValue if and only if it threw": it opens the exception-validation scope JSC'svalidateExceptionChecksmode expects at a Rust/C++ boundary and converts the result toJsResult.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/cc.test.ts