Skip to content

bun:ffi: treat arguments not passed to a cc() symbol as undefined instead of reading past the call frame - #38014

Open
robobun wants to merge 6 commits into
mainfrom
farm/937aef1a/ffi-cc-arity
Open

bun:ffi: treat arguments not passed to a cc() symbol as undefined instead of reading past the call frame#38014
robobun wants to merge 6 commits into
mainfrom
farm/937aef1a/ffi-cc-arity

Conversation

@robobun

@robobun robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Calling a cc() symbol with fewer arguments than its args declares reads past the arguments that were passed. Integer parameters receive garbage, and a pointer parameter crashes: panic(main thread): Segmentation fault at address 0x5 (debug build: AddressSanitizer: SEGV on unknown address 0x000000000005), raised from JSCELL_IS_TYPED_ARRAY inside JSVALUE_TO_PTR while the stale slot is inspected as a JSValue.
  • Cause: the C wrapper generated in Function::print_source_code (src/runtime/ffi/ffi_body.rs) emitted one *argsPtr++ per declared parameter and never looked at the call frame's argument count (LOAD_ARGUMENTS_FROM_CALL_FRAME in src/runtime/ffi/FFI.h only computed the pointer to the first argument). A host function's frame holds exactly the arguments the caller passed, so slot i >= argc is whatever the caller's stack held there.
  • The int32 family (char through u32) was passed to C as the raw JSValue bits of the slot, which is only right for an int32-tagged value: add(undefined, 1) returned 11 (the 0xa undefined tag truncated to an int), and a double-encoded number (anything JSC does not tag as int32) arrived as garbage. Substituting undefined for a missing argument is only meaningful if these parameters decode it.
  • test/js/bun/ffi/cc.test.ts already carried an it.skip for this ("looks like b defaults to 0, is this U.B.?"). It is: the value depends on what the frame happens to hold.

Fix

  • FFI.h: LOAD_ARGUMENTS_FROM_CALL_FRAME also reads argumentCountIncludingThis from the frame, and the new ARGUMENT(i) yields the slot's bits when i < argsCount and the bits of undefined otherwise. This is the fixing line; it is the C equivalent of JSC::CallFrame::argument(i), which is what the engine-backed dlopen()/linkSymbols() FFI uses (FFICallHost.cpp), so a missing argument now means the same thing for both kinds of symbols: it is undefined, and converts exactly like an explicitly passed undefined.
  • ffi_body.rs: every parameter is loaded as EncodedJSValue argN; argN.asInt64 = ARGUMENT(N); and passed through its type's to_c conversion, so the int32 family goes through JSVALUE_TO_INT32 (0 for undefined, truncation for double-encoded numbers) like every other type; ABIType::needs_a_cast_in_c is deleted. bun:ffi: fix cc() integer argument decoding and the int32 boundary in FFI.h #37978 makes this same routing change for its own reasons; whichever lands second drops the duplicate hunk. napi_env parameters still occupy their position in the JS argument list, as before; they just no longer declare an unused local.
  • The load is a member store on purpose. TinyCC emits a memset call for every brace-initialized local (tccgen.c init_putz) and, on every target except x86_64, a memmove call for a struct/union copy (tccgen.c vstore; TCC_TARGET_NATIVE_STRUCT_COPY is x86_64-only). The wrapper is compiled with -nostdlib and CompilerRT::inject provides memset but not memmove, so a union copy fails to link on aarch64 and a brace initializer costs a call per argument. Compiling an add(i32, i32) wrapper with the vendored TinyCC confirms the three shapes: union copy references memmove on arm64, brace initializer references memset on both targets, member store references neither. memmove is deliberately still not injected: a union copy in the wrapper is a per-call memmove even where it links, and the aarch64 link failure is what catches it.
  • FFI.h: JSVALUE_TO_PTR returns NULL for undefined as it already did for null. This helper is shared by the ptr, cstring, and function rows of the ABI table. For ptr/cstring that matches the engine's writePointerSlot; for function the engine throws a TypeError instead, and the wrapper has no throwing conversion yet (bun:ffi: stop cc() from turning non-numeric pointer arguments into garbage pointers #37989 adds one and routes undefined through it), so until then a missing or undefined callback reaches C as NULL rather than as the -1 pointer it decoded to before. The fixture pins the NULL so bun:ffi: stop cc() from turning non-numeric pointer arguments into garbage pointers #37989 flips that expectation deliberately.
  • ffi.cpp / ffi_body.rs: both slot indices (argumentCountIncludingThis, first argument) are taken from JSC::CallFrameSlot through the existing Bun__FFI__offsets struct. The old hardcoded BUN_FFI_POINTER_OFFSET_TO_ARGUMENTS_LIST in src/jsc/sizes.rs was that module's only content and this was its only user, so the module is deleted.
  • Cost: one compare per argument, plus, for the int32 family only, the JSVALUE_TO_INT32 call (TinyCC does not inline, so it is a real call, as the JSVALUE_TO_* call already is for every other type; previously these parameters cost nothing and decoded wrongly). Only the TinyCC-compiled cc() path is affected, not dlopen().
  • Verified:
    • test/js/bun/ffi/cc.test.ts: new spawned-fixture test covering i32/u8/bool/f64/f32/ptr/cstring/function/napi_value parameters with zero, some, all, and extra arguments, 500 repeated pointer calls from one call site, explicit undefined for the three pointer-like types, and one double-encoded integer argument; the previously skipped add(1) test now asserts the defined result. On the unfixed build the fixture reports garbage integers, non-NULL pointers, and a stale object/string coming back through napi_value; with the fix it passes under the debug ASAN build.
    • test/js/bun/ffi/ffi.test.js regenerates ffi.test.fixture.receiver.c (the checked-in copy of a generated wrapper); the regenerated file is included. The remaining suite passes except the pre-existing "all possible values" timeouts, which are dlopen() loops that exceed 5s under the debug ASAN build regardless of this change.
    • test/js/bun/ffi/cc-fixture.js (a napi_env-only signature, plus ptr/u64) still passes on the debug build.
    • cc.test.ts, ffi.test.js and test/napi/napi-value-ffi.test.ts pass on the Linux aarch64 lanes (glibc and musl), the targets where the linker enforces the union-copy constraint.

Not changed here: a missing buffer argument now takes the same path as an explicit undefined, which JSVALUE_TO_TYPED_ARRAY_VECTOR already dereferences today (the engine throws a TypeError; #37989 brings that to the wrapper), and i64/u64 parameters given undefined still hit the slow path's non-number assertion (tracked separately). In both cases this PR converts the out-of-frame read into the existing explicit-undefined behavior and leaves that behavior to the PRs that own it.

Background

  • cc() compiles the user's C with TinyCC and, per symbol, also compiles a small C wrapper (FFI.h followed by generated code, visible via viewSource()) that JSC installs directly as the host function for symbols.name. The wrapper receives the raw JSC::CallFrame*, converts the arguments itself, calls the user's function, and boxes the result. dlopen()/linkSymbols() symbols do not use this wrapper; they go through JSC's own FFI conversions.
  • A JSC call frame is an array of 8-byte registers: caller frame, return PC, code block, callee, argumentCountIncludingThis (count stored in the low 32 bits of its slot), this, then the arguments that were actually passed. Host functions are not padded to their declared length, so the frame has exactly argc argument slots; CallFrame::argument(i) is the accessor that substitutes undefined beyond that.
  • Bun__FFI__offsets (src/jsc/bindings/ffi.cpp) is how the wrapper already learns JSC layout facts (typed array vector/length offsets, the JSCell type byte): C++ fills the struct from JSC headers, and the runtime turns the fields into preprocessor defines when it compiles FFI.h.
  • The wrapper's TinyCC state is created with -nostdlib, which also disables TinyCC's fallback of resolving symbols against the host process, so CompilerRT::inject (memset, memcpy, the 64-bit slow paths, the NAPI handle scope hooks) is the complete set of symbols the wrapper can reference. The user's own C is compiled in a separate state without -nostdlib, which is why struct assignment in user code links on aarch64 while the same thing in the wrapper does not.
Earlier revisions of this PR
  • The first push had ARGUMENT(i) return an EncodedJSValue and initialized each argN from it (a union copy). Every Linux aarch64 lane failed with unresolved reference to 'memmove'; x86_64 was unaffected because TinyCC open-codes struct copies there.
  • The second revision used EncodedJSValue argN = { .asInt64 = ARGUMENT(N) };, which links everywhere but makes TinyCC zero the local through a memset call before the store, on every target and for every argument. The current revision uses the plain member store, which the generator already used for the last argument before this change.

no test proof · iteration 1 · 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

…ndefined

The C wrapper compiled for each cc() symbol loaded one call-frame slot per
declared parameter without consulting the frame's argument count, so a call
with fewer arguments than the symbol declares handed C whatever the caller's
stack held past the last argument (garbage integers, and a segfault for
pointer parameters while the stale slot was inspected as a JSValue).

The wrapper now reads argumentCountIncludingThis from the call frame and
loads each parameter with ARGUMENT(i), which yields undefined for slots past
the count, the same thing CallFrame::argument(i) does for the dlopen()
FFI. Every argument goes through its type's conversion (the int32-family
types used to be passed as raw JSValue bits), and JSVALUE_TO_PTR maps
undefined to NULL like the engine's pointer conversion does.

Both call-frame slot indices now come from JSC::CallFrameSlot through
Bun__FFI__offsets instead of the hardcoded copy in src/jsc/sizes.rs, whose
only user this was.
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8a0fb46c-412f-4a7e-9588-df6b648f2d4f

📥 Commits

Reviewing files that changed from the base of the PR and between bdb7382 and 7e738e1.

📒 Files selected for processing (8)
  • src/jsc/bindings/ffi.cpp
  • src/jsc/lib.rs
  • src/jsc/sizes.rs
  • src/runtime/ffi/FFI.h
  • src/runtime/ffi/abi_type.rs
  • src/runtime/ffi/ffi_body.rs
  • test/js/bun/ffi/cc.test.ts
  • test/js/bun/ffi/ffi.test.fixture.receiver.c

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:06 PM PT - Aug 13th, 2026

@robobun, your commit 7e738e1 has 1 failures in Build #94733 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 38014

That installs a local version of the PR into your bun-38014 executable, so you can run:

bun-38014 --bun

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on the released 1.4.0 binary and on a debug ASAN build of main (a28fcc4): a cc() symbol declared args: ["ptr"] and called with no arguments segfaults (Segmentation fault at address 0x5), and args: ["i32", "i32"] called with one argument returns garbage. Repro is the spawned fixture in test/js/bun/ffi/cc.test.ts ("calling a symbol with fewer arguments than it declares"); on the unfixed build it reports garbage integers, non-NULL pointers, and a stale object/string arriving through a napi_value parameter.

Fix is in this PR: the generated wrapper now reads the frame's argument count and substitutes undefined for arguments that were not passed, the way CallFrame::argument(i) does for dlopen() symbols. test/js/bun/ffi/cc.test.ts and test/js/bun/ffi/ffi.test.js pass on the debug build (the regenerated ffi.test.fixture.receiver.c is included).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Since it changes how the TinyCC-generated wrapper reads the raw JSC CallFrame layout and alters user-visible argument-conversion semantics for the int32 family, a human look would still be worthwhile.

What was reviewed:

  • ARGUMENT(i) bounds check and argumentCountIncludingThis payload read against JSC's little-endian slot layout — matches CallFrame::argument().
  • napi_env-only signatures no longer emit LOAD_ARGUMENTS_FROM_CALL_FRAME; verified the call-site emitter for NapiEnv doesn't reference the undeclared argN local.
  • BufferLength (whose to_c is now unreachable!()) is rejected up front for cc() at ffi_body.rs:2227, so it can't reach the new unconditional to_c() path.
  • bun_jsc::sizes has no remaining references.
Extended reasoning...

Overview

This PR fixes a memory-safety bug in bun:ffi's cc() path: the TinyCC-compiled C wrapper for each symbol read one call-frame slot per declared parameter without checking argumentCountIncludingThis, so calling with fewer arguments than declared read stale stack contents (garbage integers, or a segfault when the stale slot was inspected as a JSCell in JSVALUE_TO_PTR). The fix threads the argument count from JSC::CallFrameSlot through the existing Bun__FFI__offsets struct into the generated wrapper, adds an ARGUMENT(i) macro that substitutes ValueUndefined past argsCount, and routes every parameter through its type's to_c conversion (deleting the raw int64 pass-through for the int32 family and ABIType::needs_a_cast_in_c). JSVALUE_TO_PTR now returns NULL for undefined as it already did for null. The now-empty src/jsc/sizes.rs module is deleted.

Security risks

The bug being fixed is itself a stack over-read; the fix strictly narrows what the wrapper reads. No new attack surface is added — the two new offsets flow from JSC headers via the same Bun__FFI__offsets mechanism the wrapper already trusts for typed-array offsets.

Level of scrutiny

High. This is hand-crafted C that indexes a raw JSC::CallFrame by pointer arithmetic, and it changes user-visible behavior: the int32 family previously received raw JSValue bits (so add(undefined, 1) returned 11), and now goes through JSVALUE_TO_INT32 (returning 1). That is the correct behavior and matches the dlopen() path, but it is an API-behavior change that a maintainer should sign off on. The napi_env positional-index handling is preserved but by a different mechanism (the arg loop skips declaring a local; to_c for NapiEnv ignores its symbol name), which I traced through both emitter loops.

Other factors

Test coverage is thorough — a spawned fixture exercises i32/u8/bool/f64/f32/ptr/cstring/napi_value with none/some/all/extra arguments plus 500 repeated pointer calls, and the previously-skipped add(1) test is unskipped with a defined expectation. The regenerated ffi.test.fixture.receiver.c is included. I confirmed bun_jsc::sizes has no other consumers, that BufferLength (whose to_c hits unreachable!()) is rejected before code-gen for cc(), and that a napi_env-only signature compiles without the undeclared argsPtr/argsCount locals since ToCFormatter for NapiEnv doesn't reference them.

…n arm64

ARGUMENT(i) produced an EncodedJSValue, and initializing the argN locals
from it is a union copy. TinyCC only open-codes struct copies on x86_64; on
every other target it emits a call to memmove, which the wrapper cannot
resolve because it is linked with -nostdlib, so every cc() symbol failed to
compile on aarch64 with "unresolved reference to 'memmove'". ARGUMENT(i)
now yields the raw int64_t bits and the generated code stores them through
a designated initializer, the same shape the wrapper used before.
Comment thread src/runtime/ffi/FFI.h Outdated
Comment thread src/runtime/ffi/FFI.h Outdated
Comment thread src/runtime/ffi/FFI.h Outdated
Comment thread src/runtime/ffi/ffi_body.rs Outdated
Comment thread src/runtime/ffi/FFI.h Outdated
Comment thread src/runtime/ffi/FFI.h Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and found no bugs. The fix is well-reasoned (mirrors CallFrame::argument(i), sources both slot indices from JSC headers via the existing Bun__FFI__offsets mechanism), and the test coverage is thorough. Because it changes generated FFI wrapper code that reads raw JSC call-frame slots and alters how the int32-family arguments are decoded, a human look is worthwhile.

Checked: the Rust Offsets struct field order/types match FFIFields in ffi.cpp; argumentCountIncludingThis payload extraction via .asBits.payload is the low 32 bits on little-endian as JSC stores it; napi_env no longer declares an unused local but its to_c doesn't reference argN; sizes.rs/needs_a_cast_in_c have no remaining users; BufferLength (which has no to_c_macro) is already rejected for cc() so the unified to_c path can't hit it.

Extended reasoning...

Overview

Fixes a crash/UB where calling a cc() symbol with fewer arguments than declared reads past the JSC call frame's argument slots. Touches src/runtime/ffi/FFI.h (adds argsCount extraction and ARGUMENT(i) macro, makes JSVALUE_TO_PTR treat undefined as NULL), src/runtime/ffi/ffi_body.rs (unifies argument loading through ARGUMENT(i) and to_c, sources slot indices from Bun__FFI__offsets), src/jsc/bindings/ffi.cpp (adds two CallFrameSlot indices to the offsets struct plus a static_assert on sizeof(Register)), src/runtime/ffi/abi_type.rs (deletes needs_a_cast_in_c), and deletes src/jsc/sizes.rs (its only constant is now sourced from JSC headers). Adds a comprehensive spawned-fixture test and regenerates the checked-in wrapper fixture.

Security risks

None new. This closes an out-of-bounds stack read that could hand C garbage pointers or leak stale stack contents through napi_value. The int32-family decode change (raw bit truncation → JSVALUE_TO_INT32) is strictly a hardening: it now handles double-encoded and non-number JSValues instead of passing tag bits.

Level of scrutiny

High. This is TinyCC-compiled C that indexes a raw JSC::CallFrame* by slot offset with no engine-side safety net; getting the layout wrong is memory corruption. The fix itself is conceptually simple (bounds-check against the frame's own count), but the surrounding code — asBits.payload endianness, the (size_t*) slot arithmetic, the union-vs-scalar TinyCC lowering noted in the comment — is exactly the kind of thing REVIEW.md flags for careful review.

Other factors

I confirmed no remaining users of bun_jsc::sizes or needs_a_cast_in_c. The to_c path is now applied uniformly to every arg type; I checked that NapiEnv's to_c ignores its arg_name (so skipping the local declaration is safe) and that BufferLength — the only other type with no to_c_macro — is rejected before this codegen runs. The if any(|arg| *arg != NapiEnv) guard correctly skips LOAD_ARGUMENTS_FROM_CALL_FRAME when only napi_env args are declared, avoiding an unused-variable diagnostic. The comment-cop inline threads are all resolved (comments were subsequently shortened). Given this is not a simple/mechanical change and sits on a memory-safety-critical path, I'm deferring rather than approving.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the automated bug-hunt found no issues. The fix looks correct and well-tested, but since it reads the JSC CallFrame layout directly via pointer arithmetic and changes the generated TinyCC wrapper for every cc() symbol, a human look would still be worthwhile.

Checked: the Rust Offsets struct field order/types match the C++ FFIFields additions and Offsets::get() populates them before use; argsCount is read from the low-32-bit payload of the argumentCountIncludingThis slot with - 1 for this, matching JSC::CallFrame::argumentCount(); ARGUMENT(i) yields an int64_t and the designated initializer avoids the aarch64 TinyCC memmove lowering; napi_env no longer declares an unused local but its to_c formatter emits the env constant without referencing argN, and its position still counts toward the JS index; BufferLength is already rejected for cc() so the removed needs_a_cast_in_c branch cannot reach the unreachable!() in ToCFormatter; sizes.rs had no remaining users (the sizegen.cpp reference is a stale codegen tool that isn't consumed).

Extended reasoning...

Overview

This PR fixes an out-of-bounds read in the bun:ffi cc() path: the TinyCC-compiled wrapper that JSC installs as a host function loaded one call-frame slot per declared parameter without checking how many arguments the caller actually passed, so a call with fewer arguments read stale stack memory (garbage integers, and a segfault when a stale slot was interpreted as a cell pointer inside JSVALUE_TO_PTR). The fix reads argumentCountIncludingThis from the frame and substitutes undefined for missing slots, mirroring JSC::CallFrame::argument(i) and the engine-backed dlopen() path. Along the way it routes every argument through its to_c conversion (deleting the raw-bits pass-through for the int32 family and the now-unused needs_a_cast_in_c), makes JSVALUE_TO_PTR treat undefined as NULL like null, sources both frame slot indices from JSC::CallFrameSlot via the existing Bun__FFI__offsets struct, and deletes the hardcoded sizes.rs.

Security risks

Low. This is the FFI boundary, but the change strictly narrows what memory the wrapper reads (it stops reading past the frame) and does not expose any new surface. The new static_assert(sizeof(JSC::Register) == sizeof(void*)) guards the pointer-arithmetic assumption. JSVALUE_TO_PTR(undefined) → NULL matches the engine's writePointerSlot and the pre-existing null handling.

Level of scrutiny

This is native, ABI-sensitive code that indexes a raw JSC::CallFrame* by slot offsets and generates C that TinyCC compiles at runtime — the PR itself already hit one platform-specific failure (union copy → memmove on aarch64) between revisions. The reasoning is sound and the test coverage is thorough (spawned fixture across i32/u8/bool/f64/f32/ptr/cstring/napi_value with none/some/all/extra args, plus a 500-iteration JIT-tier loop), but per the repo's review guidance native memory-safety changes are the most-scrutinized category and this warrants a human maintainer's eyes on the CallFrame layout assumptions and the behavioral choice that missing args now silently become undefined rather than throwing.

Other factors

All comment-cop bot threads (paragraph-long comments) were addressed in 991b2c2/6dae8ea5 and are resolved. I confirmed sizes.rs had no other consumers (the only remaining reference is in the unused sizegen.cpp codegen tool's stdout string). BufferLength is rejected before this codegen path runs, so the collapsed to_c-for-everything call-site loop cannot reach unreachable!(). The Offsets struct's new fields are appended in the same order/types as the C++ FFIFields, and Offsets::get() calls Bun__FFI__ensureOffsetsAreLoaded before reading them. The regenerated ffi.test.fixture.receiver.c matches the new emitter output.

…unction and double-encoded arguments

TinyCC zero-fills every brace-initialized local through a call to memset,
so the designated initializer cost one memset call per argument on every
target. Declaring the local and assigning the member, the shape the
generator already used for the last argument, references neither memset
nor memmove (checked by compiling the wrapper with the vendored TinyCC for
x86_64 and arm64).

The arity fixture now also covers a "function" parameter, which shares
JSVALUE_TO_PTR with ptr/cstring and therefore receives NULL for a missing
or undefined callback, and one double-encoded integer argument, which the
removed raw pass-through decoded wrongly.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes FFI codegen and reads raw JSC CallFrame slots directly (with a subtle behavioral change to how the int32 argument family decodes), a human look would still be worthwhile.

What was reviewed:

  • FFIFields (C++) and Offsets (Rust) field order/types match; CallFrameSlot::argumentCountIncludingThis and argumentOffset(0) yield the same slot indices the old hardcoded 6 implied.
  • ARGUMENT(i) is only expanded with integer literals, so no macro double-eval hazard; argsCount sign/width vs. the literal is fine.
  • NapiEnv no longer declares an argN local, and to_c(NapiEnv) ignores the arg name so the call site still compiles; the any(!= NapiEnv) gate on LOAD_ARGUMENTS_FROM_CALL_FRAME is consistent with that.
  • sizes.rs / needs_a_cast_in_c have no remaining references.
Extended reasoning...

Overview

Fixes an out-of-bounds read in the TinyCC-compiled cc() wrapper: previously each declared parameter unconditionally read *argsPtr++ from the JSC call frame, so calling with fewer arguments than declared read stale stack slots (garbage integers, or a segfault in JSVALUE_TO_PTR). The fix threads argumentCountIncludingThis from JSC::CallFrameSlot through the existing Bun__FFI__offsets struct, adds an ARGUMENT(i) macro that substitutes TagValueUndefined past argsCount, and unifies argument loading so every type (including the int32 family) goes through its to_c conversion. JSVALUE_TO_PTR now treats undefined as NULL. sizes.rs (whose only content was the now-superseded hardcoded slot index) and ABIType::needs_a_cast_in_c are deleted.

Security risks

None identified. This narrows behavior from reading uninitialized/stale stack memory to a defined value, which is strictly safer. No new untrusted-input parsing.

Level of scrutiny

High. This is FFI/ABI code that directly indexes into JSC's CallFrame register layout, and the wrapper is JIT-installed as a raw host function. It also changes user-visible argument semantics for the int32 family (raw slot bits → JSVALUE_TO_INT32), which is a bug fix but still a behavior change. The aarch64/TinyCC memmove interaction required two revisions to land on the member-store form. These are exactly the kinds of low-level, cross-platform details a maintainer familiar with the FFI subsystem should sign off on.

Other factors

The test coverage is strong — a spawned fixture exercises i32/u8/bool/f64/f32/ptr/cstring/function/napi_value with 0/some/all/extra args, a 500-iteration JIT-tier loop, explicit undefined, and a double-encoded integer; the previously-skipped add(1) test is un-skipped with a defined result. The regenerated ffi.test.fixture.receiver.c is included. All comment-cop feedback about long comments was addressed and resolved. The Offsets struct layout matches FFIFields field-for-field, and grep confirms no remaining users of the deleted sizes module or needs_a_cast_in_c. Given the subsystem and the semantic change, deferring to a human is the right call despite finding no defects.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes how the cc() wrapper indexes into raw JSC::CallFrame slots and reworks the int32-family argument decode path (with TinyCC/aarch64-specific codegen constraints that already required two revisions), a human look is still worthwhile.

What was reviewed:

  • ARGUMENT(i) bounds check and argsCount derivation from CallFrameSlot::argumentCountIncludingThis — matches JSC::CallFrame::argument(i) semantics; the payload read on little-endian is correct.
  • Offsets struct layout in ffi_body.rs mirrors FFIFields in ffi.cpp field-for-field; the deleted sizes.rs had no other referents.
  • NapiEnv still occupies its JS argument index without declaring a local; BufferLength is rejected before codegen so the unreachable!() arm in ToCFormatter stays unreachable.
  • The int32-family now routes through JSVALUE_TO_INT32, which handles double-encoded values and undefined correctly; u32 values above INT32_MAX still round-trip via the int64_t intermediate.
Extended reasoning...

Overview

This PR fixes an out-of-bounds read in the TinyCC-compiled wrapper that cc() generates per symbol: the wrapper previously loaded one call-frame slot per declared parameter without checking how many arguments the caller actually passed, so a short call handed C garbage integers and, for pointer parameters, segfaulted while inspecting a stale slot as a JSValue. The fix reads argumentCountIncludingThis from the frame (via a new field on the existing Bun__FFI__offsets struct populated from JSC::CallFrameSlot) and substitutes the encoded bits of undefined for indices past the count — the C equivalent of CallFrame::argument(i). The generator now uniformly emits EncodedJSValue argN; argN.asInt64 = ARGUMENT(N); and passes every argument through its to_c conversion, which also fixes the int32 family's decode of double-encoded numbers. JSVALUE_TO_PTR now returns NULL for undefined. The dead needs_a_cast_in_c() and the single-constant src/jsc/sizes.rs module are deleted.

Security risks

The fix eliminates a stack over-read (reading past the JSC call frame's argument slots), which is strictly a hardening improvement. No new attack surface is introduced; the wrapper still runs user-provided C, which is the existing trust boundary.

Level of scrutiny

High. This is FFI/ABI code that indexes directly into the raw JSC::CallFrame register array by slot number, with a static_assert on sizeof(JSC::Register) but otherwise relying on JSC layout constants. The generated wrapper is compiled by TinyCC under -nostdlib, and the PR history shows two earlier revisions failed on Linux aarch64 due to TinyCC emitting memmove/memset calls for union copies and brace-initialized locals — the current member-store form was chosen specifically to avoid both. The int32-family argument decode is a behavior change (raw bits → JSVALUE_TO_INT32), and while it is strictly more correct, it affects every cc() call with an int-family parameter. Cross-platform FFI with target-specific linker constraints falls squarely under REVIEW.md's "Cross-platform" and "Verify semantics empirically" sections.

Other factors

The test coverage is thorough: a spawned fixture exercises i32/u8/bool/f64/f32/ptr/cstring/function/napi_value with zero/some/all/extra arguments, 500 repeated calls to walk JIT tiers, explicit undefined, and a double-encoded integer; the previously-skipped add(1) test is un-skipped with a defined expectation; ffi.test.fixture.receiver.c is regenerated. The PR description states aarch64 CI lanes pass. All comment-cop feedback (paragraph-long comments) has been addressed and resolved. I confirmed BufferLength is rejected in reject_cc_unsupported_types_error before print_source_code runs, so the _ => unreachable!() arm in ToCFormatter::fmt remains unreachable for the arg direction. The Offsets Rust struct and C++ FFIFields struct match field-for-field. No remaining references to the deleted bun_jsc::sizes module exist in the tree.

Given the sensitivity of raw call-frame indexing, the platform-specific TinyCC codegen constraint that already bit twice in this PR's own history, and the behavior change to int32-family argument decoding, this deserves human sign-off even though the automated review found no defects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant