bun:ffi: make cc() reject non-numeric i64/u64 arguments instead of passing garbage - #38057
bun:ffi: make cc() reject non-numeric i64/u64 arguments instead of passing garbage#38057robobun wants to merge 3 commits into
Conversation
…ssing garbage The wrapper cc() compiles for a symbol converts i64, u64, i64_fast and u64_fast arguments with JSVALUE_TO_INT64 / JSVALUE_TO_UINT64 in FFI.h. Their slow paths were JSC__JSValue__toInt64 and JSC__JSValue__toUInt64NoTruncate, which assert that the value is a number or a BigInt; for anything else (undefined, null, a boolean, an object, a string) debug builds abort on that assertion and release builds hand C INT64_MIN or 0. The slow path is now Bun__FFI__jsValueToInt64Slow, which runs the engine's writeSlotFromJSValue for the argument's type, the conversion dlopen()/linkSymbols()/CFunction arguments already get. BigInts convert as before; everything else throws the engine's TypeError. The generated wrapper converts these arguments into locals before the call and returns the empty JSValue as soon as one of them threw, so the native function is never entered with an exception pending; the napi handle scope, when there is one, is opened after these conversions. Apart from that reordering in napi wrappers, wrappers without 64-bit integer arguments generate the same C as before.
|
Warning Review limit reached
Next review available in: 13 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 |
|
Status: fix and tests pushed (reworked after self-review, see the "Earlier revision" note in the description), waiting on CI. Reproduced on 1.4.0 with an |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the FFI/JSC bridge (runtime-generated C, host-function exception protocol, and a new extern "C" slow path), a human look would still be worthwhile.
What was reviewed:
to_c()call sites — both are inprint_source_codeand either declarethrewbeforehand or branch onarg_conversion_can_throw()first;print_callbackgenerates no C source.- Handle-scope reordering — throwing conversions now run before
NapiHandleScope__open, so the earlyreturn ValueEmptyhas nothing to close; slot loads andwriteSlotFromJSValuecreate no napi handles. ABI_TYPE_*defines are added inFunction::compile(wrapper only), not the user'sCompileC::compile; the removedJSVALUE_TO_UINT64_SLOWsymbol is no longer declared inFFI.h.
Extended reasoning...
Overview
This PR fixes cc()-compiled FFI wrappers so that i64/u64/i64_fast/u64_fast arguments given non-numeric values throw a TypeError (matching dlopen()/CFunction) instead of aborting debug builds or passing garbage in release. It touches: a new extern "C" bridge (Bun__FFI__jsValueToInt64Slow in JSCFFIBridge.cpp) that delegates to the engine's writeSlotFromJSValue and reports whether it threw; signature changes to JSVALUE_TO_INT64/JSVALUE_TO_UINT64 in the TinyCC-compiled FFI.h; ABI-table and wrapper-codegen changes in abi_type.rs/ffi_body.rs so throwing conversions run into locals first and the wrapper returns ValueEmpty before entering C or opening a napi handle scope; ~230 lines of new tests plus a regenerated viewSource() fixture.
Security risks
None new. The change replaces an unchecked slow path (which read arbitrary JSValues as doubles) with a checked one that throws for non-numbers/BigInts. The generated wrapper now correctly follows the JSC host-function convention on throw (return the empty JSValue with the exception pending). The ffiContext() accessor and writeSlotFromJSValue are the same machinery dlopen() already uses.
Level of scrutiny
High. This is native FFI/JSC bridge code: runtime-compiled C that reads raw NaN-boxed call-frame slots, a new C++/Rust ABI boundary, and control-flow changes in generated wrappers around JSC exception handling and napi handle scopes. Getting the throw protocol or handle-scope ordering wrong would be a memory-safety or exception-scope-verification failure. While the change is well-designed and thoroughly tested (matrix over four types × fourteen inputs × two entry points, a two-arg call counter proving C isn't entered, a napi_env variant, and BUN_JSC_validateExceptionChecks=1), it is not simple or mechanical enough for auto-approval.
Other factors
The PR description explicitly notes textual overlap with three other open FFI.h PRs (#37978, #37989, #38014) in print_source_code and the ABI table — a maintainer should be aware of the merge/rebase order. I verified that the only two to_c() call sites are both correctly guarded (the pre-conversion loop declares threw; the call-site loop uses converted{i} for throwing types), that the callback direction doesn't generate C source, and that the removed JSVALUE_TO_UINT64_SLOW TinyCC symbol has no remaining reference in FFI.h.
The slow path is now one JSVALUE_TO_SLOT_SLOW (Bun__FFI__jsValueToSlotSlow) that takes the argument's type tag and returns the engine's argument slot; the FFI.h conversions cast it. Every ABIType row carries its ABI_TYPE_* define name and says whether its conversion can throw, so the generator derives the tag defines and the up-front conversion loop from the table and another type only needs its row changed to use the same mechanism. Tests: the two-argument function now takes (i64, u64) so the TypeError shows which argument was rejected, and a (i32, i64, f64) function covers a fallible conversion between inline ones. The matrix comment spells out that the inline number branches are not what is being compared.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the cc() wrapper code-gen (new ToC::Fallible mechanism, moved napi handle-scope open, a new C++ bridge into writeSlotFromJSValue) and is one of four overlapping FFI.h fixes that need to land in a coordinated order, a human look is still warranted.
What was reviewed:
ToCFormatter'sFalliblearm is only reached from the pre-conversion loop wherebool threwis in scope; the call-site loop routes fallible args toconverted{i}first.define_symbolssignature matchestag_defines()'s[(&str, i64); 22]; the defines are added only inFunction::compile, not in the user's C compilation.- Early return on throw happens before
NapiHandleScope__open, so no scope leaks;BufferLengthreachingunreachable!()in theSpecialarm is pre-existing, not introduced here. Bun__FFI__jsValueToSlotSlowusesDECLARE_THROW_SCOPE+scope.exception()and only writes*threwon the throw path (the wrapper zero-initialises it).
Extended reasoning...
Overview
The PR routes the slow path of cc()'s 64-bit integer argument conversion through the engine's JSC::FFI::writeSlotFromJSValue (the same converter dlopen()/CFunction use), so non-numeric/non-BigInt inputs now throw the engine's TypeError instead of aborting debug builds or passing garbage in release. Mechanically: FFI.h's JSVALUE_TO_INT64/UINT64 gain (globalObject, abiType, &threw) params and tail-call a new JSVALUE_TO_SLOT_SLOW symbol bound to Bun__FFI__jsValueToSlotSlow; abi_type.rs gains a ToC enum (Infallible/Fallible/Special) plus per-row tag_define names; print_source_code in ffi_body.rs now converts fallible arguments into locals before the native call, checking threw after each and returning ValueEmpty on throw, and opens the napi handle scope only after conversions succeed. CompilerRT::inject swaps the two removed slow-path symbols for the new one, and Function::compile #defines all 22 ABI_TYPE_* tags. The regenerated ffi.test.fixture.receiver.c reflects the header change; the two new cc.test.ts blocks cover the input matrix against both cc() and CFunction, multi-arg ordering, native-call suppression, mixed fallible/infallible signatures, and the napi_env bail-out.
Security risks
None identified. The change tightens input validation (rejecting inputs that previously passed garbage to native code), does not expose new surface, and the ABI-type tag passed to the engine converter is a compile-time constant baked into the generated C, not user-controlled.
Level of scrutiny
High. This is FFI code-generation and JSC exception plumbing across a TinyCC-compiled boundary — a mistake here can crash or corrupt callers of any cc() symbol with an i64/u64 parameter, and the wrapper C is compiled at runtime so type mismatches are not caught by the host toolchain. The change also introduces a table-driven mechanism (ToC::Fallible) explicitly intended as the shared foundation for three other open PRs (#37978, #37989, #38014); a maintainer should confirm this is the shape they want before those PRs rebase onto it.
Other factors
The tests are unusually thorough (matrix comparison against CFunction over the same C, native-call counter, napi_env variant) and the description documents verified failure on the unfixed binary and success under debug ASAN with BUN_JSC_validateExceptionChecks=1. All comment-cop bot findings were addressed in da48000 and are marked resolved. I checked that removing the WORKAROUND struct fields for the old slow paths leaves the remaining two int64→JSValue functions intact, that the Special arm's unreachable!() for BufferLength is unchanged from before, and that the moved handle-scope open cannot skip a matching close (the only early return is before it opens). The remaining reasons to defer are the design sign-off on the Fallible mechanism and merge ordering with the three overlapping PRs, not correctness concerns.
…39124) ### Problem - `ABIType::param_typename` (src/runtime/ffi/abi_type.rs:229) had the same signature and body as `ABIType::typename` (abi_type.rs:220): both write `self.typename_label()` to the writer. A change to one would silently miss the other. This is the `reimplemented_helper` finding baselined for `abi_type.rs` in `mordant-baseline.toml`. - The duplication is inherited, not a porting mistake. `paramTypename` was added in #7009 already delegating to `typenameLabel()`, the same as `typename`. The variant that would have differed (`paramTypenameLabel`, which spelled `uint32_t` parameters as `int32_t`) was never called by anything and was dropped as dead code when `ffi.zig` was ported. Large `uint32_t` arguments do not depend on it: `print_source_code` loads every small integer argument as the raw `int64_t` encoding and passes it to a prototype whose parameter type (`uint32_t`) truncates it to the low 32 bits, so the parameter and return positions have always used the same C type name. ### Fix - Delete `param_typename`; the only caller (`Function::print_source_code` in src/runtime/ffi/ffi_body.rs, the prototype's parameter list) now calls `typename`. - Remove the `reimplemented_helper:src/runtime/ffi/abi_type.rs` entry from `mordant-baseline.toml` and drop the mention of the never-ported `param_typename_label` from the table comment. - No behavior change. Verified with the debug build: - `viewSource` prototype for a symbol taking all 20 argument types is byte-identical to the one the released `bun` emits (see below). - `bun bd test test/js/bun/ffi/ffi.test.js --timeout 120000`: 157 pass (the `--timeout` is only because the exhaustive integer identity tests exceed the 5s default under a debug+ASAN build; CI passes its own larger per-test timeout). - `bun bd test test/js/bun/ffi/cc.test.ts test/js/bun/ffi/ffi-viewSource-non-object.test.ts test/js/bun/ffi/ffi-error-messages.test.ts`: 35 pass. - The `mordant` workflow runs on this PR since the baseline changed; with the entry removed, it fails if the finding is still reported. - #38057 (open) adds another `param_typename` caller in `ffi_body.rs`; whichever of the two lands second needs that one call renamed to `typename`. ### Background - `bun:ffi`'s `cc()` / `dlopen()` fallback path generates a C wrapper per symbol and compiles it with the bundled TinyCC. `print_source_code` emits the prototype of the user's function (`<ret> name(<param> arg0, ...)`) and the wrapper that unpacks `EncodedJSValue`s and calls it; `typename` supplies the `<ret>` and `<param>` spellings from the per-variant `ABI_TABLE`. - `mordant` is the dylint pack run by `bun run rust:mordant`; `mordant-baseline.toml` holds per-(lint, file) counts of pre-existing findings, and CI fails only on findings above the baseline. <details> <summary>Prototype comparison, released bun vs this branch</summary> ```js import { viewSource } from "bun:ffi"; const types = ["char","int8_t","uint8_t","int16_t","uint16_t","int32_t","uint32_t","int64_t","uint64_t","double","float","bool","ptr","cstring","i64_fast","u64_fast","function","napi_env","napi_value","buffer"]; const [src] = viewSource({ f: { args: types, returns: "uint32_t" } }, false); ``` Both binaries emit: ```c /* --- The Function To Call */ uint32_t f(char arg0, int8_t arg1, uint8_t arg2, int16_t arg3, uint16_t arg4, int32_t arg5, uint32_t arg6, int64_t arg7, uint64_t arg8, double arg9, float arg10, bool arg11, void* arg12, void* arg13, int64_t arg14, uint64_t arg15, void* arg16, napi_env arg17, napi_value arg18, void* arg19); ``` </details>
|
Heads up: #39124 (merged, 454b1ca) removed |
Problem
cc()that declares ani64,u64,i64_fastoru64_fastargument and is given something that is neither a number nor a BigInt (undefined,null, a boolean, an object, a string) aborts debug builds withASSERTION FAILED: value.isHeapBigInt() || value.isNumber()inJSC__JSValue__toInt64(src/jsc/bindings/bindings.cpp:4366); release builds hand the C functionINT64_MIN(signed types) or0(unsigned types).cc()compiles converts these arguments withJSVALUE_TO_INT64/JSVALUE_TO_UINT64(src/runtime/ffi/FFI.h:314and:329on main). Both decode int32-tagged and double-encoded numbers inline and tail-call a slow path for everything else; the slow paths wereJSC__JSValue__toInt64andJSC__JSValue__toUInt64NoTruncate(bindings.cpp:4363,:4468), which only accept numbers and BigInts and read anything else as a double. The wrapper also had no way to report a failed conversion: it always went on to call the C function.cc()is affected. Since bun:ffi: use the engine-native FFI when available #35246,dlopen()/linkSymbols()/CFunctionconvert arguments in the engine (JSC::FFI::writeInt64Slot), which converts int32/double/BigInt and throwsTypeError: bun:ffi cannot convert argument to 'i64'(the type's name) for anything else.cc()still calls through the TinyCC-compiled wrapper built fromFFI.h.FFI.hfixes: bun:ffi: fix cc() integer argument decoding and the int32 boundary in FFI.h #37978 (int32-family arguments and the int32 box boundary), bun:ffi: stop cc() from turning non-numeric pointer arguments into garbage pointers #37989 (pointer-typed arguments) and bun:ffi: treat arguments not passed to a cc() symbol as undefined instead of reading past the call frame #38014 (missing arguments). bun:ffi: treat arguments not passed to a cc() symbol as undefined instead of reading past the call frame #38014 makes a missingi64argument behave like an explicitundefined, i.e. it lands on this path.Fix
FFI.h: the int32 and double branches ofJSVALUE_TO_INT64/JSVALUE_TO_UINT64are unchanged; their slow path is nowJSVALUE_TO_SLOT_SLOW(globalObject, abiType, &threw, value), bound to the newBun__FFI__jsValueToSlotSlow(JSCFFIBridge.cpp), which runs the engine'swriteSlotFromJSValuefor the argument's type, returns the 64-bit argument slot and sets*threwwhen the engine threw. The twobindings.cppfunctions keep their other callers and are no longer handed to TinyCC.abi_type.rs: each row of the ABI table now carries itsABI_TYPE_*define name and says whether its conversion isInfallible(f(arg), as before) orFallible(f(JS_GLOBAL_OBJECT, ABI_TYPE_X, &threw, arg)); the four 64-bit integer rows areFallible.Function::compiledefines every tag for the wrapper's compilation only (the user's C, compiled byCompileC::compile, does not see them), andprint_source_codeconverts everyFallibleargument into a local before the call, returning the empty JSValue (the host-function convention after a throw) as soon as one threw, so the C function is never entered with an exception pending; the napi handle scope, when there is one, is opened after these conversions. Another argument type that wants the same treatment only needs its row flipped toFallibleplus its ownFFI.hinline function; bun:ffi: stop cc() from turning non-numeric pointer arguments into garbage pointers #37989 (pointer types) is that case, and this is the mechanism both fixes share.dlopen()has run since bun:ffi: use the engine-native FFI when available #35246), so the values this change routes to it, BigInts and non-numbers, now get exactlydlopen()'s results anddlopen()'s TypeErrors; BigInts produce the same bits as before (toBigInt64/toBigUInt64either way), so the only observable change is that inputs which previously aborted or produced garbage now throw. Numbers are not routed and their inline branches are untouched. That is deliberate:u64's double branch already differs from the engine for negative and non-finite numbers (TinyCC's unsigned conversion drops the sign), and the engine's owndoubleToUInt64mis-converts[2^63, 2^64)wherecc()is right, so the two need one decision made together; that is tracked as a separate bug and this PR does not claim parity for them. The other pre-existingcc()-only extra, a TypedArray given tou64/u64_fastconverting to its length, is likewise untouched and pinned by the test so the difference fromdlopen()stays visible.test/js/bun/ffi/ffi.test.fixture.receiver.cis the checked-inviewSource()output 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.print_source_codeand the ABI table; whichever lands later rebases (bun:ffi: fix cc() integer argument decoding and the int32 boundary in FFI.h #37978 and bun:ffi: treat arguments not passed to a cc() symbol as undefined instead of reading past the call frame #38014 deleteneeds_a_cast_in_c(), which the call-site branch here still consults; bun:ffi: stop cc() from turning non-numeric pointer arguments into garbage pointers #37989 becomes fourFalliblerows plus itsFFI.hand test changes on top of this).test/js/bun/ffi/cc.test.ts, new64-bit integer argumentsblock. The matrix test runs the same inputs (numbers, BigInts, eight kinds of non-numbers) through acc()wrapper and through aCFunctionover the same C functions for all four types and requires the tables to match; an(i64, u64)function shows which argument is reported when several are bad (left to right, like the engine) and, via a call counter, that a rejected argument stops the call before C; an(i32, i64, f64)function covers a fallible conversion between inline ones. A second test covers the bail-out in anapi_envwrapper. Both fail on the unfixed binary (release: garbage values and 8 native calls instead of 4 in thecctable while theenginetable already matches; debug: the assertion abort above) and pass under the debug ASAN build, also withBUN_JSC_validateExceptionChecks=1.bun bd test test/js/bun/ffi/: everything else passes except the pre-existing 5s timeouts of thedlopen()"integer identities" tests and the two worker-terminate tests, which take about 5s each under debug ASAN in this container and do not usecc().test/js/bun/ffi/cc-fixture.js(uint64_targument plusnapi_env) still passes on the debug build.Background
cc()compiles the user's C with TinyCC and, per symbol, compiles a second small C file:FFI.hfollowed by a wrapper generated byFunction::print_source_code(viewSource()prints it). JSC installs that wrapper directly as the host function forsymbols.name; it reads the raw NaN-boxed argument slots from the call frame, converts them, calls the user's function and boxes the result.FFI.his therefore C compiled at runtime, and anything it calls (add_symbol) or any constant it uses (define_symbols) has to be handed to TinyCC byCompilerRT::inject/Function::compileinffi_body.rs.undefined,null, booleans) or a pointer to a heap cell (objects, strings, BigInts).FFI.hcan tell numbers apart by their tag bits; a BigInt and a plain object are both just cells to it, which is why everything that is not a number goes to a slow path in the runtime.JSC::FFI::writeSlotFromJSValue(engine,FFIConversions.cpp) converts one JS argument into the 64-bit slot the native call thunk reads, given the argument'sFFI::Type.ABITypeinabi_type.rsuses the same numbering asFFI::Type(pinned by thestatic_asserts inJSCFFIBridge.cpp), so the tag the wrapper passes can be handed to the engine as is.ValueEmptyhere), and the caller checks the VM's exception slot after the call. Running native code or further conversions with an exception pending is not allowed, hence the check after each conversion.Repro from the report
Debug build before the fix:
null,true,{},"12",() => 1and a TypedArray (fori64) behave the same way asundefinedbefore and after; numbers and BigInts are unchanged. The same definitions throughCFunctionover the compiled function's address throw the TypeErrors shown above both before and after.Earlier revision
The first revision (8cb84c3) had the same fix but wired it specifically to the 64-bit integer types: an int64-only bridge function, a hand-written list of four tag defines, and a hard-coded list of throwing types. Self-review pointed out that #37989 needs the identical plumbing for pointer types, that the two-argument test could not tell which argument had thrown (both were
i64), and that the body over-claimed parity foru64doubles. 66691e3 made the mechanism table-driven as described above, changed the multi-argument tests, and scoped the parity claim; da48000 shortened comments.