bun:ffi: fix cc() integer argument decoding and the int32 boundary in FFI.h - #37978
bun:ffi: fix cc() integer argument decoding and the int32 boundary in FFI.h#37978robobun wants to merge 3 commits into
Conversation
…c() trampoline cc() symbols still go through the TinyCC-compiled trampoline generated from FFI.h, which converted integers differently from the engine-native path used by dlopen()/linkSymbols(): - char/i8/u8/i16/u16/i32/u32 arguments were passed as the raw NaN-boxed JSValue bits truncated to the parameter width, which is only right when the engine happened to int32-tag the number. Any double-encoded argument (>= 2^31, fractional, or produced by JIT-compiled arithmetic) reached C as garbage: identity_u32(0xFFFFFFFF) returned 4292870144, identity_u32(2**31) returned 0, undefined reached C as 10. - MAX_INT32 was 2^31, one past INT32_MAX, so a u32 or i64_fast return of exactly 2^31 was boxed as the int32 -2147483648. Route every argument through its FFI.h conversion (the JSVALUE_TO_INT32 entries in the ABI table were never emitted because needs_a_cast_in_c() excluded these types) and make JSVALUE_TO_INT32 apply ECMAScript ToInt32 to double-encoded values, ported from JSC's toIntImpl<int32_t>, so cc() now matches the engine's toInt32()/toUInt32() for every number. Fix MAX_INT32 and add MIN_INT32 for the 64-bit fast-path boxing helpers.
|
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 (5)
Comment |
|
Status: reproduced on 1.4.0-canary ( Review so far: the automated reviews had no bug findings; the follow-ups from self-review (booleans, the per-argument memset, the in-tree parity test) and the comment-length threads are addressed in d5a4a8d. Previous build 93792 was green on all 179 lanes that ran ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites JSVALUE_TO_INT32 in the TinyCC trampoline header as a bit-level port of JSC's toIntImpl<int32_t> and reworks how print_source_code emits every argument, a human look would still be worthwhile.
What was reviewed:
- Hand-checked the new ToInt32 bit manipulation at 2^31, 2^32-1, and the NaN/Infinity/undefined/cell paths (all reach the
exp > 83zero); shift amounts stay in-range for their operand widths. - Confirmed
UINT32_TO_JSVALUE/INT64_TO_JSVALUE/UINT64_TO_JSVALUEnow box exactly[INT32_MIN, INT32_MAX]as int32 with the correctedMAX_INT32/MIN_INT32. - Verified folding
NapiValueinto the commonEncodedJSValuepath is equivalent (ToCFormatteremitsargN.asNapiValue),BufferLengthis rejected before codegen soto_c()'sunreachable!()stays unreachable, andneeds_a_cast_in_chas no remaining callers.
Extended reasoning...
Overview
This PR fixes integer argument decoding and int32-boundary return boxing in the bun:ffi cc() path only. It touches the TinyCC trampoline header (src/runtime/ffi/FFI.h), the C-source generator (src/runtime/ffi/ffi_body.rs print_source_code), removes the now-dead needs_a_cast_in_c from abi_type.rs, adds two focused tests to cc.test.ts, and regenerates the viewSource() fixture. The core change is that every argument is now loaded as an EncodedJSValue and routed through its FFI.h conversion macro (previously the seven ≤32-bit integer types passed raw NaN-boxed bits), and JSVALUE_TO_INT32's double-encoded branch is replaced by an ECMAScript ToInt32 port from JSC's MathCommon.h.
Security risks
None identified. This is JS-number → C-integer conversion inside a user-compiled trampoline; a wrong result is a wrong integer, not a memory-safety issue. The new code path does no allocation, no pointer arithmetic on user-controlled sizes, and cannot re-enter JS. Cell-typed JSValues passed where an integer is expected now decode to 0 rather than truncated pointer bits, which is strictly safer.
Level of scrutiny
Medium-high. FFI.h is compiled into every cc() symbol's trampoline, so a subtle mistake in the ToInt32 bit-twiddling would silently corrupt data across the entire cc() surface. I hand-verified the algorithm at the boundaries the tests exercise (2^31 → -2147483648, 0xFFFFFFFF → -1, NaN/±∞/undefined/null/cells all hit the exponent-1024 early-zero) and confirmed every shift stays within its operand width, but the port is ~20 lines of IEEE-754 bit manipulation that a maintainer familiar with this subsystem should sign off on. The MAX_INT32 and codegen-simplification pieces are straightforward on their own.
Other factors
The verification story is unusually strong: the author reports an 18,335-call cc()-vs-dlopen() comparison with 0 mismatches after and 18,173 before, and the new tests are designed so that argument-decoding and return-boxing bugs cannot mask each other (C reports received integers back as double). The Rust simplification correctly folds the NapiValue special case into the common path (its ToCFormatter arm emits argN.asNapiValue, which needs argN to be an EncodedJSValue — which it now always is), BufferLength is rejected in reject_cc_unsupported_types_error before codegen so the unreachable!() in ToCFormatter stays unreachable, and grep confirms needs_a_cast_in_c has no remaining callers. The new test block is not skipIf(isASAN), matching the adjacent double <-> JSValue conversions block that already runs cc() under ASAN via subprocess; the PR description states it was verified passing under debug ASAN. No prior human reviews or outstanding comments.
|
Updated 7:28 AM PT - Aug 13th, 2026
❌ @robobun, your commit d5a4a8d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37978That installs a local version of the PR into your bun-37978 --bun |
There was a problem hiding this comment.
I reviewed this and found no bugs. Given the hand-ported IEEE-754 ToInt32 bit manipulation in JSVALUE_TO_INT32 (compiled by TinyCC at runtime) and the three outstanding comment-cop inline comments on FFI.h, a human sign-off is worthwhile.
What was reviewed
JSVALUE_TO_INT32: traced exp<0, exp>83 (NaN/±Inf/undefined/null/cell → 0), exp=31 (2^31 → 0x80000000), exp=32..52 (implicit-1 wraps to 0, exponent bits stay above the low-32 window), exp=53..83 shift — matches ECMAScript ToInt32 and JSC'stoIntImpl.MAX_INT32/MIN_INT32and the<=change inUINT64_TO_JSVALUE/INT64_TO_JSVALUE/UINT32_TO_JSVALUE: the int32 box now covers exactly [INT32_MIN, INT32_MAX].ffi_body.rs: the removedint64_tpassthrough andNapiValuebranch collapse into the unconditionalEncodedJSValueload +arg.to_c();BufferLength(whoseto_cisunreachable!()) is still rejected byreject_cc_unsupported_types_erroron every caller ofprint_source_code, so no new panic path.
Extended reasoning...
Overview
This PR fixes bun:ffi cc() integer argument decoding and the int32 return-boxing boundary. It touches src/runtime/ffi/FFI.h (the C header that TinyCC compiles into per-symbol trampolines), the Rust code generator src/runtime/ffi/ffi_body.rs (print_source_code), src/runtime/ffi/abi_type.rs (deletes the dead needs_a_cast_in_c() helper), adds two tests to test/js/bun/ffi/cc.test.ts, and regenerates the viewSource() fixture. Only cc() is affected — dlopen/linkSymbols/CFunction/JSCallback go through the engine's FFI since #35246.
Security risks
None identified. The change tightens argument conversion (previously, raw NaN-boxed bits reached C as truncated garbage; now they go through ToInt32). Non-number cells now become 0 instead of truncated pointer bits, which is strictly safer. No new user-controlled data reaches native memory operations.
Level of scrutiny
Medium-high. FFI is memory-safety-adjacent (wrong integer values passed to C can index buffers in user code), and JSVALUE_TO_INT32 is now ~15 lines of dense IEEE-754 exponent/mantissa manipulation compiled by TinyCC at runtime. I traced the algorithm against ECMAScript ToInt32 for the exp boundary cases (0, 31, 32, 52, 83, 84, NaN/Inf) and it matches; the PR reports an 18,335-call cross-validation against dlopen() with 0 mismatches, and the new huge/huge_negative test cases pin the behavior the old (int32_t)(int64_t)double cast got wrong. But this is the kind of code where a maintainer should confirm they want the JSC-ported bit-twiddling rather than, say, calling into a shared helper.
Other factors
- Outstanding review comments: three
comment-cop(github-actions) inline comments on FFI.h lines 255/262/268 flag the multi-line explanatory comments inJSVALUE_TO_INT32. These are automated lint on comment length; the comments document a non-obvious ported algorithm and cite the source, which is arguably the case where such comments are warranted, but the author hasn't responded to or trimmed them. ffi_body.rssimplification: I confirmed the removed special cases (NapiValue, the last-arg two-statement form) are subsumed by the new unified branch, and thatBufferLength— the only arg type whoseToCFormatterhitsunreachable!()— is rejected byreject_cc_unsupported_types_erroron all three call paths toprint_source_code(cc() at ffi_body.rs:1036, JSCallback at :1308, viewSource at :1415), so the unconditionalarg.to_c()introduces no new panic path.- Test coverage: strong. Argument and return sides are tested independently (args reported back as
doubleso a return-boxing bug can't mask an argument bug), boundary values on both sides, and the description confirmsUSE_SYSTEM_BUN=1fails 19/26 + 4/10 cases. - The robobun status comment already relayed that a prior automated pass suggested human review of the ToInt32 port; I'm not repeating that recommendation for its own sake, but combined with the unaddressed comment-cop comments and the FFI subsystem, I'm not comfortable auto-approving.
…gument, add engine parity test JSVALUE_TO_INT32 returns 1 for JSValue(true), as ToInt32 does, so booleans match the engine path like the other immediates. The generated trampoline assigns each argument slot instead of using an aggregate initializer, which TinyCC compiles to a memset() call per argument. The new test calls the same C functions through cc() and through CFunction over a corpus that covers each branch of the decode and requires identical results. Comments in FFI.h are trimmed to one line each.
|
Follow-ups from review, pushed in d5a4a8d:
The description is updated to match. |
Problem
cc()mis-converts integers at and above the int32 boundary. Withuint32_t identity_u32(uint32_t x) { return x; }compiled throughcc():identity_u32(0xFFFFFFFF)returns4292870144,identity_u32(0x80000000)returns0, and a function returning0x80000000uyields-2147483648. The same definitions throughdlopen()return the right values (repro in the details block).src/runtime/ffi/abi_type.rsneeds_a_cast_in_c()excludedchar/i8/u8/i16/u16/i32/u32, soprint_source_code(src/runtime/ffi/ffi_body.rs) emittedint64_t arg0 = *argsPtr;and passed the raw NaN-boxed JSValue bits to the C parameter, which truncated them. That is only right when the engine int32-tagged the number. Any double-encoded argument (outside int32 range, fractional, or produced by JIT-compiled arithmetic:identity_u32(x + 0.5 - 0.5)returns0) reached C as the low 32 bits of a double encoding, andundefined/null/truereached C as their tag bits (10/2/7). TheJSVALUE_TO_INT32(entries in the ABI table for these types were never emitted.src/runtime/ffi/FFI.hhad#define MAX_INT32 2147483648(2^31, one pastINT32_MAX), soUINT32_TO_JSVALUEandINT64_TO_JSVALUE(u32andi64_fastreturns) boxed exactly 2^31 as the int32-2147483648.cc()is affected. Since bun:ffi: use the engine-native FFI when available #35246,dlopen()/linkSymbols()/CFunction/JSCallbackuse the engine's FFI and convert correctly;cc()still calls through the TinyCC-compiled trampoline built fromFFI.h. ffi: u32 and i64_fast returns of 2 ** 31 arrive in JS as -2147483648 #33340 and bun:ffi: infer exact signatures from symbol definitions, fix cc argument/return conversions #32075 described the same symptoms and were closed as superseded by bun:ffi: use the engine-native FFI when available #35246, which did not reach this path.Fix
print_source_codeloads every argument as anEncodedJSValueand passes it through the type'sFFI.hconversion;needs_a_cast_in_c()and the rawint64_tpassthrough are deleted. This is the shape every other argument type already used (JSVALUE_TO_DOUBLE,JSVALUE_TO_INT64,JSVALUE_TO_PTRall branch on the encoding); the passthrough only worked because the JS wrapper layer removed in bun:ffi: use the engine-native FFI when available #35246 forcedval | 0before the call. The exact fixing lines are the now unconditionalwrite!(writer, "{}", arg.to_c(arg_name))and#define MAX_INT32 2147483647.JSVALUE_TO_INT32's non-int32 branch is ECMAScript ToInt32, ported from JSC'stoIntImpl<int32_t>(runtime/MathCommon.h), plustrue -> 1. The engine path converts these seven types withJSValue::toInt32()/toUInt32(), socc()now hands C the same value asdlopen()for every number,undefined,nulland both booleans. The(int32_t)(int64_t)doublecast it replaces is undefined in C for |x| >= 2^63 and differs between x64 and arm64 there.toUInt32is literallytoInt32); the generated prototype'suint32_t/uint8_t/... parameter performs the narrowing, exactly as the engine'sstatic_cast<uint8_t>(truncated)does.EncodedJSValue argN; argN.asInt64 = ...;(the form the old code used for its last argument) rather than an initializer, because TinyCC compiles a local aggregate initializer into amemset()call (vendor/tinycc/tccgen.c,init_putz). Disassembling the JIT'd trampoline for(u32, f64) -> u32shows 4 calls with this PR versus 3 before: the added one isJSVALUE_TO_INT32, which is the cost of the fix; it is the same out-of-line decodef64andptrarguments already pay.MAX_INT32is nowINT32_MAXandMIN_INT32is added, so the int32 box is used for exactly[INT32_MIN, INT32_MAX]inUINT32_TO_JSVALUE,UINT64_TO_JSVALUEandINT64_TO_JSVALUE.u64_fastreturns exactly2**53 - 1as a BigInt in both paths (the engine has the same<comparison); strings, objects and BigInts passed to int parameters, which the engine path rejects or converts by calling into the VM, reach C as0(previously truncated pointer bits). The trampoline has no way to call back into the VM for those.test/js/bun/ffi/cc.test.ts, newinteger <-> JSValue conversionsblock (runs under ASAN; all three tests fail on the released build and pass withbun bd test):doubles so a return-boxing bug cannot mask or mimic an argument bug; includes the int32 boundaries, wraparound, fractions, a JIT double-encoded value,1e19(exercises the exponent > 52 branch; the old cast gets it wrong too),2**84, NaN/Infinity,undefined/null/true/false.cc()trampoline and once throughCFunction(the engine's FFI, pointed at the addresses the C code returns) over a 60-value corpus covering every branch of the decode, 420 calls, must agree exactly. On the released build this reports the double-encoded and immediate cases as mismatches.u32,i64_fastandu64_faston both sides of the int32 boundary.test/js/bun/ffi/ffi.test.fixture.receiver.cis theviewSource()output thatffi.test.js("ffi print") rewrites; regenerated by running it. Its generated body is unchanged from main; only the embedded header differs.bun bd test test/js/bun/ffi/: everything passes except the pre-existinginteger identities work for all possible values64-bit cases inffi.test.js, which aredlopen()tests that take 4 to 6 seconds against a 5 second timeout under debug ASAN in this container and are unrelated to this change.Background
true,false,undefinedandnullare small tag constants (7, 6, 10, 2).bun:ffihas two call paths.dlopen()/linkSymbols()/CFunctionhand the signature to JavaScriptCore's FFI, which converts arguments and returns in C++ (FFIConversions.cpp).cc()instead generates a small C wrapper per symbol (FFI.hplus a few lines fromprint_source_code), compiles it with the bundled TinyCC, and installs it as the host function;viewSource()prints that wrapper. The conversions inFFI.hare the only conversions acc()call gets, and TinyCC does not inline or optimize them.truebecomes 1. ToUint32 is the same operation with the result read as unsigned. It is whatInt32Array/Uint32Arraystores,x | 0, and JSC'sJSValue::toInt32()all compute.Repro
Before (1.4.0 canary):
After, both lines read
4294967295 2147483648 7 0 2147483648.Changes made during review
The first revision returned 0 for
true(the engine returns 1), emitted the argument slot as an aggregate initializer (amemset()call per argument under TinyCC), and only had the hand-written table as coverage. d5a4a8d adds theTagValueTruecheck, switches to assignment, adds thecc()-vs-CFunctionparity test, and trims theFFI.hcomments to one line each. A local 18,335-call comparison againstdlopen()on the same shared library also had 0 mismatches (18,173 before the fix); the committed parity test is the in-tree version of that check.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