Conversation
…nstead of assuming a double The C wrapper cc() compiles converted ptr, cstring and function arguments with JSVALUE_TO_PTR, which handled null, typed arrays and int32 and then treated anything else as a double. A JSCallback object, an ArrayBuffer, a BigInt, an object with a `ptr` property, undefined, or a plain object became a garbage pointer (-1); for a function argument that is an immediate call through address -1. The buffer row read the typed array vector offset off whatever it was given. The inline paths now cover numbers, views and null (for ptr/cstring); every other value is handed to JSC::FFI::writeSlotFromJSValue through a new JSVALUE_TO_PTR_SLOW export, so cc() wrappers accept and reject the same values dlopen()'d symbols do. The generated wrapper converts these arguments into locals before the call and returns the empty value when a conversion threw, so the native function is not called with a pending exception; the napi handle scope is opened after the conversions for the same reason.
|
Warning Review limit reached
Next review available in: 14 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 (6)
Comment |
|
Updated 5:05 AM PT - Aug 13th, 2026
⏳ @robobun, your commit 366b833 is still building in
|
|
Status: ready for review at head 366b833. CI build 94710 passed on every lane that ran (Linux glibc and musl, x64 and aarch64, the ASAN lane, Windows x64 and aarch64); the one red test there, Reproduced on the released binary ( Self-review follow-ups since the first push: the generated wrapper shape is now pinned by a Two pre-existing problems found while probing are not part of this change and have been filed separately: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches FFI pointer conversion and JSC exception propagation in TinyCC-generated host functions, and includes a deliberate behavior change (null/undefined for a function argument now throw where they previously passed NULL), a human look would still be worthwhile.
What was reviewed:
to_cis only called fromprint_source_code, so the newJS_GLOBAL_OBJECT/&threwprefixes are always in scope; both TCC compilation paths (~808 and ~2073 in ffi_body.rs) callCompilerRT::define/injectso the newABI_TYPE_*defines andJSVALUE_TO_PTR_SLOWsymbol are available.- The napi handle scope move: pointer conversions run before
NapiHandleScope__open, so the earlyreturn ValueEmptyhas nothing to close. JSVALUE_TO_PTRhas no other callers (checked repo-wide); the callback-direction fixture doesn't use it.Bun__FFI__jsValueToPointerSlowuses a throw scope and only writes*threwon exception;slotis zero-initialized.
Extended reasoning...
Overview
The PR routes cc()'s pointer/cstring/function/buffer argument conversion through the same engine converter (JSC::FFI::writeSlotFromJSValue) that dlopen()/CFunction use. It touches the TinyCC-compiled FFI header (FFI.h), the Rust code generator (print_source_code in ffi_body.rs, ABI_TABLE in abi_type.rs), a new C++ bridge (Bun__FFI__jsValueToPointerSlow), and adds ~290 lines of table-driven tests comparing cc() against CFunction over the same C functions.
Security risks
The unfixed code decoded arbitrary JSValues as garbage pointers and called them, so the change is a strict hardening. The new slow path delegates to the existing engine converter and throws for unrecognized inputs rather than passing through. No new attack surface identified; the null-callback case is now a TypeError instead of a NULL call.
Level of scrutiny
High. This is memory-safety-adjacent FFI code: raw NaN-boxed JSValue decoding, exception propagation from a TinyCC-compiled host function back into JSC, and a code generator that emits C compiled at runtime. Errors here manifest as segfaults or silent pointer corruption. It also carries a small but real user-visible behavior change (null/undefined function args now throw to match dlopen()).
Other factors
The change is well-scoped, thoroughly documented, and the test compares cc() output against CFunction output over a full input matrix, plus a napi_env bail-out case. The regenerated fixture and static_assert guards for enum drift look correct. I verified the two TCC compilation paths both receive the new defines/symbol, that to_c has no callers outside print_source_code, and that no other consumers of JSVALUE_TO_PTR exist. Nonetheless, given the surface area (codegen + runtime-compiled C + JSC exception discipline) and the deliberate behavior change, this warrants a maintainer's eyes rather than automated approval.
|
On the one point the review raises: The red "Source lints" check is |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the ABI_TYPE_* namespace-leak concern in 06b076a — the defines are now scoped to Function::compile() only, and the enum { ABI_TYPE_PTR, ... } line in the test fixture's user C pins that. This run found no further issues. Given this touches FFI pointer conversion, JSC exception propagation from TinyCC-compiled C, and reorders the napi handle-scope open, a human look is still worthwhile.
What was reviewed:
to_c_macrocall sites — onlyprint_source_codeuses it, so the newJS_GLOBAL_OBJECT/&threwprefixes have no other consumers.CompilerRT::inject/definecall sites —POINTER_TAG_DEFINESis now wrapper-only;JSVALUE_TO_PTR_SLOWsymbol injection into the user's TCC state is a linker symbol, not a macro, so harmless.JSVALUE_TO_PTRreordering — int32/double checks precede the cell check;JSCELL_IS_TYPED_ARRAY(null)is false (OtherTag hits NotCellMask), so no null deref before the explicit null branch.Bun__FFI__jsValueToPointerSlow— throw-scope declared, exception checked before returning,*threwset on the exception path only.
Extended reasoning...
Overview
The PR fixes cc()-compiled FFI wrappers so that pointer-typed arguments (ptr, cstring, function, buffer) that aren't a number/view/null are routed through the engine's writeSlotFromJSValue instead of being blindly decoded as a NaN-boxed double (which produced garbage pointer -1 and a segfault for function args). Files: FFI.h (new slow-path fallback in JSVALUE_TO_PTR, new JSVALUE_TO_BUFFER), JSCFFIBridge.cpp (new Bun__FFI__jsValueToPointerSlow extern calling writeSlotFromJSValue), abi_type.rs (updated to_c_macro prefixes, new POINTER_TAG_DEFINES, new arg_conversion_can_throw), ffi_body.rs (codegen now emits per-argument conversion locals with bail-out on throw, moves napi handle-scope open after conversions, injects the new symbol/defines), plus ~350 lines of tests and the regenerated fixture.
Prior feedback addressed
My previous review flagged that ABI_TYPE_* macros were being #defined into the user's C via the shared CompilerRT::define(). Commit 06b076a moved state.define_symbols(ABIType::POINTER_TAG_DEFINES) out of CompilerRT::define and into Function::compile() right after the wrapper's CompilerRT::define(state) call (option 2 from my suggestion), and added enum { ABI_TYPE_PTR, ABI_TYPE_CSTRING, ABI_TYPE_FUNCTION, ABI_TYPE_BUFFER }; to the test fixture's user-side pointers.c so the test would fail if the leak reappears. Fully resolved.
Security risks
FFI pointer conversion is memory-safety-adjacent, but this change strictly narrows the failure surface: inputs that previously produced a garbage pointer now either take the same inline path they always did (int32/double/typed-array/null) or delegate to the engine converter that dlopen() already runs, which throws a TypeError for anything it doesn't recognize. The one deliberate divergence (JS strings for cstring throw instead of transcoding, because there's no arena to free the copy) is a safety improvement over the prior garbage pointer. No new attack surface identified.
Level of scrutiny
High — this is generated C compiled at runtime by TinyCC, calling back into JSC with a raw JSGlobalObject*, propagating exceptions via a bool* out-param and the empty-JSValue return convention, and reordering when the napi handle scope opens. A mistake here is a process crash or an exception-scope violation. The change looks correct (throw-scope declared, exception checked, bail-out returns ValueEmpty.asZigRepr before entering C, handle scope opened only after all conversions succeed), the tests are thorough (matrix comparison against CFunction over the same C, per-argument conversion counting via a .ptr getter, native-call counter proving rejected calls never reach C, viewSource snapshot pinning wrapper shape, napi bail-out test), and the author reports it passes under BUN_JSC_validateExceptionChecks=1 and ASAN. But the surface area and the JSC-integration subtlety warrant a maintainer's eyes.
Other factors
to_c()/ToCFormatterhas no other call sites beyondprint_source_code, so the newJS_GLOBAL_OBJECT, ABI_TYPE_*, &threw,prefixes can't appear in a context lacking those identifiers.CompilerRT::inject(which now adds theJSVALUE_TO_PTR_SLOWlinker symbol) runs for both the user's C and the wrapper — that's fine; it's a symbol, not a macro.- The comment-cop bot's flags were addressed in a6a512b (comments shortened to one line each).
- The PR description notes textual overlap with #37978 in the same generator loop; that's a merge-order concern for a maintainer, not a correctness one.
…lling helpers TinyCC ignores always_inline, so every JSVALUE_IS_* / JSCELL_IS_TYPED_ARRAY helper used by JSVALUE_TO_PTR was a real call. Test the tag bits directly in the one function, fold the buffer variant into it, and handle undefined like null for ptr/cstring inline (the engine's isUndefinedOrNull test) so only values the inline code cannot convert leave the wrapper. Accepted inputs now cost the JSVALUE_TO_PTR call alone. The double path converts through int64 like the engine's doubleToInt64 instead of an unsigned conversion.
There was a problem hiding this comment.
I re-reviewed after 06b076a and 366b833 and didn't find any bugs. My earlier concern (the ABI_TYPE_* defines leaking into the user's C) is addressed — the define_symbols call now sits in Function::compile only, and the matrix fixture's enum guards it. Because this touches hand-decoded JSC tag bits in TinyCC-compiled C plus a new exception-propagation path through the generated wrapper, a human look is still worthwhile.
What was checked this run:
to_c/ToCFormatterhas no other call site, so the new&threw/JS_GLOBAL_OBJECTprefix is only emitted where those names are in scope.- The
(bits & ~UndefinedTag) == TagValueNullfast path matches onlynull/undefined;true/falsefall through to the slow path (test coverstrue). CompilerRT::inject(which now addsJSVALUE_TO_PTR_SLOW) runs on both the wrapper and user-C states, so the symbol resolves; the tag defines run only for the wrapper.
Extended reasoning...
Overview
The PR fixes cc()-compiled FFI wrappers turning non-numeric pointer/cstring/function/buffer arguments into garbage pointers (segfault on a JSCallback object, -1 for anything else). It adds a slow path (Bun__FFI__jsValueToPointerSlow in JSCFFIBridge.cpp) that delegates to the engine's writeSlotFromJSValue — the same converter dlopen() uses since #35246 — and rewires the generated C wrapper (print_source_code in ffi_body.rs) to convert pointer-typed arguments into locals before the call, bailing out with ValueEmpty after each one if it threw. The napi handle scope now opens after the conversions so a bail-out has nothing to unwind. FFI.h's JSVALUE_TO_PTR keeps inline fast paths for int32/double/typed-array-view/null-undefined and hands everything else to the slow path.
Security risks
None new. The change strictly narrows what reaches C: inputs that previously became a garbage pointer now either take the same inline path or throw a TypeError. The new slow path re-enters JS (a .ptr getter), and the wrapper correctly checks threw after each conversion before continuing — the test verifies conversion stops at the first failing argument and the getter behind it never runs.
Level of scrutiny
High. This is native FFI code that hand-decodes JSC's NaN-boxed EncodedJSValue in TinyCC-compiled C (cell mask, int32 tag, double offset, immediate tags), dereferences cell pointers to read the JSType byte, and adds a new C++ bridge and a new exception-return convention through generated C. Per the repo's review guidelines, native memory safety is the most-blocked category, and this sits squarely in it.
Other factors
- My earlier inline finding (the
ABI_TYPE_*defines being injected viaCompilerRT::defineand thus visible in the user's own.c) was fixed in 06b076a by moving thedefine_symbolscall intoFunction::compile, and the fixture now declares an enum with those names to pin it. - The one still-open inline comment is the comment-cop bot re-flagging
abi_type.rs:147, which is theABI_TABLErow-label alignment (not a comment block); the author already explained this on the earlier identical flag. - Test coverage is thorough: a
viewSourceinline snapshot pins the wrapper shape, a matrix fixture runs the same input set throughcc()andCFunctionand requires the tables to match, per-argument conversion count and stop-on-first-failure are asserted via a counting getter, and a separate napi_env fixture checks the handle-scope ordering. - I confirmed
to_c()has no other callers (the callback-direction generator usesto_js), so the widened macro prefix cannot land in a scope that lacksthrew/JS_GLOBAL_OBJECT.
This branch has not been deployed
1 participant
Problem
cc()that declares a"function"argument crashes the process when it is given aJSCallbackobject instead ofcallback.ptr:panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFF. The same declaration throughdlopen()accepts either form, and the docs present passing the object as the normal form (.ptris described as a performance tip).cc()compiles convertsptr,cstringandfunctionarguments withJSVALUE_TO_PTR(src/runtime/ffi/FFI.h:210 on main), which handles null, typed arrays and int32 and then treats every other value as a NaN-boxed double. A JSCallback, an ArrayBuffer, a BigInt, an object with aptrproperty,undefined, a plain object or a string all decode to the pointer-1. For afunctionargument the C code calls it; forptr/cstringit is handed to C as a valid-looking pointer.bufferrow (src/runtime/ffi/abi_type.rs:146 on main) has the same shape of bug: it reads the typed-array vector slot out of whatever it is given, so a number ornullsegfaults and anArrayBufferor plain object passes a garbage pointer.cc()is affected. Since bun:ffi: use the engine-native FFI when available #35246,dlopen()/linkSymbols()/CFunctionconvert arguments in the engine (JSC::FFI::writeSlotFromJSValue), which handles all of these values and throws a TypeError for the rest.cc()still calls through the TinyCC-compiled wrapper built fromFFI.h. ffi: wrap JSCallback objects passed as cc() function arguments #31776 addressed this crash in the JS wrapper layer and was closed when bun:ffi: use the engine-native FFI when available #35246 removed that layer; the wrapper path it did not cover is the one fixed here.Fix
FFI.h:JSVALUE_TO_PTRnow takes the argument's type tag and decides the unambiguous inputs itself: typed arrays and DataViews for all four types; int32, double, and null/undefined (as NULL) forptr/cstring; int32 and double forfunction. Everything else goes to a newJSVALUE_TO_PTR_SLOW, which isBun__FFI__jsValueToPointerSlow(JSCFFIBridge.cpp) calling the engine'swriteSlotFromJSValuefor that type: that is where a JSCallback, ArrayBuffer, BigInt or{ ptr }object is converted, where a null/undefined callback and a non-viewbufferget the engine's TypeErrors, and where junk is rejected.always_inline, so the existingJSVALUE_IS_*/JSCELL_IS_TYPED_ARRAYhelpers are real calls in the compiled wrapper. The tag tests inJSVALUE_TO_PTRare therefore written out on the raw bits, and every accepted input costs the oneJSVALUE_TO_PTRcall. On main the same inputs cost 1 call for null, 5 or 6 for a number and 6 for a view, and abufferargument was one unchecked vector read; only values the engine has to look at leave the wrapper now. The double path converts throughint64like the engine'sdoubleToInt64(the old unsigned conversion was a libtcc1 helper call, and undefined for negative values).print_source_codenow converts these arguments into locals before the call and returns the empty JSValue (the host-function convention after a throw) as soon as one of them threw; the native function is never entered with an exception pending. Every napi wrapper now opens its handle scope after the argument loads and these conversions instead of before them, so the bail-out has nothing to unwind (the only change to wrappers without pointer-typed arguments; the argument loads need no scope). Wrappers with neither pointer-typed nor napi arguments generate the same C as before, which is why the regeneratedffi.test.fixture.receiver.conly changes in itsFFI.hhalf.ABI_TYPE_PTRetc.) reach the C side throughdefine_symbols, the mechanismFFI.halready uses for the JSC offsets, so the values come from theABITypeenum rather than being repeated in the header. They are defined only when the wrapper is compiled (Function::compile), not for the user's own C file, which the matrix fixture checks by declaring an enum with those names.dlopen()has run since bun:ffi: use the engine-native FFI when available #35246), so delegating to it makescc()accept the same values (JSCallback, ArrayBuffer, BigInt, objects with a numericptr,undefinedas NULL forptr/cstring) and reject the same values with the same TypeErrors, includingnull/undefinedfor afunctionargument, whichdlopen()rejects and whichcc()previously passed on as NULL (a fall-out of the untyped routine;0still passes NULL explicitly on both paths). Every input that worked before is still converted inside the wrapper to the same pointer, so the engine only ever sees inputs that previously produced a garbage pointer.cstringargument throws incc()(the engine's "a JavaScript string is not valid here" TypeError) wheredlopen()transcodes it. The engine frees the copy through an arena bracketed around the call, which the cc() wrapper does not have;cc()never accepted strings here (they became-1), so this turns a garbage pointer into an error rather than removing anything. The test pins the difference.test/js/bun/ffi/cc.test.ts("pointer-typed arguments"), three tests that all fail on the unfixed binary:viewSource()for anapi_env/function/f64/cstring/buffer/ptrsignature pins the wrapper shape: the tagged conversions into locals, the bail-out after each one, the inline conversion of thef64, and the handle scope opened only after the conversions (on the unfixed binary it shows the old inlineJSVALUE_TO_PTR(argN)calls);cc()wrapper and through aCFunctionover the same C functions and requires the two result tables to be equal (on the unfixed binary the process segfaults at the first row). A countingptrgetter shows each argument is converted exactly once and that conversion stops at the first argument that fails (the getter behind it never runs); a C-side counter shows the rejected calls never reached C; a throwing getter's exception propagates as-is;napi_envwrapper: a rejected argument does not reach C, and the inline (view, null) and slow-path ({ ptr }) inputs still work afterwards (on the unfixed binary the rejected argument reaches C as-1).bun bd test test/js/bun/ffi/(debug ASAN): everything passes except the pre-existing 5s timeouts of the dlopen "integer identities" tests on this container, which my change does not touch.BUN_JSC_validateExceptionChecks=1.FFI.hat runtime, one debug binary was timed calling a one-argumentcc()symbol in a loop with three headers swapped in (the call structure of main, the first version of this PR, and the final one); numbers are in the details block. The final header is at or below main's structure for every input class; the first version of this PR was slower than main for null, views andbufferand sentundefinedthrough the engine, which is what prompted the rewrite.test/js/bun/ffi/ffi.test.fixture.receiver.cis the checked-in copy of the generated source and is regenerated by theffi printtest, as in bun:ffi: decode double-encoded JSValues in JSVALUE_TO_INT32 #34653 and ffi, napi: purify NaNs before NaN-boxing doubles from native code #32787.Background
cc()compiles the user's C with TinyCC and, per symbol, also compiles a small C wrapper generated byFunction::print_source_code(src/runtime/ffi/ffi_body.rs) withFFI.hprepended. JSC calls that wrapper directly as a host function with(globalObject, callFrame); the wrapper reads the raw NaN-boxed argument slots, converts them, calls the user's function and boxes the result.FFI.his therefore C compiled at runtime, and the conversion helpers in it have to be given any runtime symbols (add_symbol) and constants (define_symbols) they use;CompilerRT::inject/definein ffi_body.rs do that.ABI_TABLEin abi_type.rs holds, per argument type, the prefix of the C expression the generator emits to convert an argument;INT64_TO_JSVALUE_SLOW(JS_GLOBAL_OBJECT,is an existing example of a prefix that names the wrapper's own parameter, which the new pointer prefixes also do (JS_GLOBAL_OBJECT,&threw).ValueEmptyhere, encoded as 0). Running further JS (such as another argument'sptrgetter) with an exception pending is not allowed, which is why the wrapper checks after each conversion rather than once at the end.JSC::FFI::writeSlotFromJSValueis the engine-side converter added for bun:ffi: use the engine-native FFI when available #35246; for pointer types it accepts null/undefined (NULL, except forfunction), int32/double, BigInt, typed arrays and DataViews, ArrayBuffers,JSFFICallbackcells (what aJSCallbackis) and objects whoseptrproperty is a number or BigInt, and throws a TypeError otherwise. Itscstringstring transcoding needs the caller to pass a string arena; passing none makes a JS string throw.static ... __attribute__((always_inline))helpers inFFI.hare compiled as ordinary functions and every use is a call, which is why the hot checks inJSVALUE_TO_PTRtest the NaN-boxing tag bits directly. The constants it uses (NumberTag,NotCellMask,UndefinedTag, the JSC type byte and vector offsets) are the same ones the helpers use.Cost check of the three headers (debug build, loaded shared host)
One-argument
cc()symbols called in a loop from JS, best of 5 x 1M calls per run, ns per call including the JS-to-host call itself; minimum over 4 runs for the first and last columns, a single run for the middle one (taken on the binary built at that commit, before the header swap setup existed). The host had a load average above 200, so only the ordering is meaningful; the structural difference (number of calls made per argument, listed in the Fix section) is deterministic.ptr, numberptr, Uint8Arrayptr, nullptr, undefinedbuffer, Uint8Arrayfunction, number"main's check structure" is main's
JSVALUE_TO_PTRbody behind this PR's signature (plus main's unchecked vector read forbuffer), swapped into the source tree for the current debug binary, which readsFFI.hfrom there at startup; the final column is the same binary with the real header.Before/after on the repro from the report
Other inputs on the unfixed binary,
ptrargument echoed back by C:undefined,{},"hello",true, an ArrayBuffer, a BigInt,{ ptr }and a JSCallback all arrive in C as-1.bufferargument: a number ornullsegfaults, an ArrayBuffer arrives as an unrelated pointer read out of the JSArrayBuffer cell. After the fix each of these behaves as it does throughdlopen().