bun:ffi: decode double-encoded JSValues in JSVALUE_TO_INT32

[robobun]

What does this PR do?

JSVALUE_TO_INT32 in src/runtime/ffi/FFI.h truncated the raw NaN-boxed asInt64, which only works when the JSValue is int32-tagged. Whether an integer-valued JS number is int32-tagged or double-encoded is the engine's choice (JIT tier, DFG double speculation, Math.round/Math.floor provenance), so an int-typed JSCallback could return the correct value for a loop's first ~12 iterations and then 0 once the callback body tiered up.

Repro

import { cc, JSCallback, FFIType as t } from "bun:ffi";
import { mkdtempSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";

const d = mkdtempSync(`${tmpdir()}/cbret-`);
writeFileSync(`${d}/cb.c`, "typedef int (*cbi)(int); int call1(cbi f, int x){ return f(x); }");
const C = cc({ source: `${d}/cb.c`, symbols: { call1: { args: [t.function, t.i32], returns: t.i32 } } }).symbols;

const cbH = new JSCallback(x => { const v = x + 0.5; return v - 0.5; }, { args: [t.i32], returns: t.i32 });
const cbF = new JSCallback(() => 5.7, { args: [t.i32], returns: t.i32 });
C.call1(cbH.ptr, 938); // 0, want 938
C.call1(cbF.ptr, 0);   // -858993459, want 5

Fix

Mirror JSVALUE_TO_DOUBLE: check JSVALUE_IS_INT32 first, otherwise subtract DoubleEncodeOffset and read asDouble. The (int64_t) intermediate cast keeps (uint32_t)JSVALUE_TO_INT32(...) defined for values in (INT32_MAX, UINT32_MAX] (the u32 return type shares this macro).

All existing callers (JSVALUE_TO_PTR, JSVALUE_TO_DOUBLE, JSVALUE_TO_INT64, JSVALUE_TO_UINT64) already guard with JSVALUE_IS_INT32 before calling this, so the added branch is a no-op for them.

A prior attempt at this area (#33095) wrapped the JS callback at construction time to coerce the return value; this change fixes the TinyCC-side decode instead, which costs nothing at call time and also covers any path that reaches JSVALUE_TO_INT32 without going through FFIBuilder.

How did you verify your code works?

New test in test/js/bun/ffi/cc.test.ts exercises i32/u32/i8/u16 JSCallback returns with double-encoded integer values (pinned via x + 0.5 - 0.5 on a runtime arg) and a fractional return. Fails on main with echo_double: 0, fractional: -858993459, etc.; passes with the fix.

---

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

[robobun]

Reproduced on main, fails-before / passes-after verified locally with bun bd test test/js/bun/ffi/cc.test.ts. All 42 ffi tests pass. Both bot review threads resolved.

Ready for review. No test/js/bun/ffi/ failures on any lane across builds 75492 and 75501. Remaining red lanes are unrelated to this diff (FFI.h is only compiled by TinyCC when bun:ffi is imported; none of the failing tests use it):

Test	Lane	Status
test-worker-message-port-transfer-terminate.js	debian x64-asan	pre-existing JSC assertion on main
node-net.test.ts	alpine aarch64	main break (mimalloc page-count threshold), owned by another fix
bake/deinitialization.test.ts	alpine x64	main break (dev_server segfault), owned by another fix
test-net-connect-memleak.js	—	main break, owned by another fix
bun-install-registry.test.ts, es-module-lexer.test.ts, package.json tarball, no-orphans, bun-jsc, webview-chrome, etc.	various	flaky (passed on retry)

[robobun]

^{Updated 11:28 AM PT - Jul 18th, 2026}

❌ @robobun, your commit 70fbcd8 has 4 failures in Build #75501 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/bake/deinitialization.test.ts - pid 4272 segmentation fault at address 0xfffffffffffffff8 on 🐧 3.23 x64
• test/js/node/net/node-net.test.ts - code 1 on 🐧 3.23 aarch64
• test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 34653

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

bun-34653 --bun

[coderabbitai]

Walkthrough

Changes

FFI integer decoding

Layer / File(s)	Summary
Integer JSValue conversion src/runtime/ffi/FFI.h, test/js/bun/ffi/ffi.test.fixture.callback.c, test/js/bun/ffi/ffi.test.fixture.receiver.c	JSVALUE_TO_INT32 now handles int32-tagged and double-encoded integer values, returning 0 for decoded NaN values.
Integer callback integration tests test/js/bun/ffi/cc.test.ts	Adds coverage for integer-returning callbacks that compute results through intermediate double arithmetic and verifies the resulting C-side values.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately describes the main change to JSVALUE_TO_INT32 decoding.
Description check	✅ Passed	The description matches the required template and includes the change summary, repro, fix, and verification details.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[github-actions]

This PR may be a duplicate of:

1. bun:ffi: coerce JSCallback return values to the declared return type #33095 - Also fixes JSCallback return values with integer types being double-encoded; uses a JS-side coercion wrapper instead of fixing the C-side JSVALUE_TO_INT32 macro
2. ffi: u32 and i64_fast returns of 2 ** 31 arrive in JS as -2147483648 #33340 - Fixes JSVALUE_TO_INT32-adjacent macros and MAX_INT32 constant in FFI.h for the same encoding/decoding boundary issues
3. fix(ffi): multiple long-standing FFI correctness bugs #31449 - Omnibus FFI correctness PR that subsumes the fixes from both bun:ffi: coerce JSCallback return values to the declared return type #33095 and ffi: u32 and i64_fast returns of 2 ** 31 arrive in JS as -2147483648 #33340, overlapping heavily with this PR's JSVALUE_TO_INT32 changes

🤖 Generated with Claude Code

[robobun]

Not duplicates:

• bun:ffi: coerce JSCallback return values to the declared return type #33095 wraps the callback at construction time on the JS side; this PR fixes the TinyCC-side JSVALUE_TO_INT32 decode instead (no per-call overhead, also covers paths that bypass FFIBuilder). Already noted in the PR body.
• ffi: u32 and i64_fast returns of 2 ** 31 arrive in JS as -2147483648 #33340 fixes the MAX_INT32 constant and INT64_TO_JSVALUE/UINT64_TO_JSVALUE boundary comparisons (C→JS direction for i64_fast/u32 at 2^31). No overlap with JSVALUE_TO_INT32.
• fix(ffi): multiple long-standing FFI correctness bugs #31449 is a broader FFI rework, but its JSVALUE_TO_INT32 is still return val.asInt64; (line 276 of its FFI.h), so the bug this PR fixes is still present there.

[claude]

LGTM — mirrors the existing JSVALUE_TO_DOUBLE int32-tag check for the symmetric case.

What was reviewed:

• Traced all JSVALUE_TO_INT32 consumers in abi_type.rs (char/i8/u8/i16/u16/i32/u32) — the (int64_t) intermediate keeps the (uint32_t) cast on the u32 path defined for values in (INT32_MAX, UINT32_MAX], and narrower types truncate correctly.
• Confirmed the in-header callers (JSVALUE_TO_PTR/_DOUBLE/_INT64/_UINT64) all pre-guard with JSVALUE_IS_INT32, so the new branch is a no-op for them.
• Test follows the neighboring spawned-fixture pattern in the same describe block; covers int32-tagged control, double-encoded integer, fractional ±, and the u32/i8/u16 codegen variants.

Extended reasoning...

Overview

Six-line change to JSVALUE_TO_INT32 in src/runtime/ffi/FFI.h (plus the two regenerated fixture .c files that embed a copy of that header) so it decodes double-encoded JSValues instead of truncating the raw NaN-boxed bits. This is the exact mirror of the fix already present in JSVALUE_TO_DOUBLE a few lines below. A new spawned-subprocess test in cc.test.ts covers i32/u32/i8/u16 JSCallback return types with a callback body that forces a double-represented integer.

Security risks

None. This is a numeric decode of a value the JS engine already produced; no untrusted input parsing, no memory dereference, no new pointer arithmetic. The change strictly narrows the set of inputs that produce garbage.

Level of scrutiny

Low-medium. The FFI header is compiled by TinyCC into per-call trampolines, so correctness matters, but the change is mechanical: it adds the same JSVALUE_IS_INT32 guard + DoubleEncodeOffset subtraction pattern used by four sibling helpers in the same file. I verified the (int32_t)(int64_t)double chain: the int64 step avoids the direct double→int32 out-of-range UB for the u32 path (3e9 fits in int64, then narrows modulo 2³² under TinyCC's 2's-complement implementation-defined behavior, then the codegen's (uint32_t) cast recovers the value). Values outside int64 range or NaN remain garbage-in-garbage-out, which is unchanged from before and out of contract for an int-typed return.

Other factors

The test is placed alongside the existing "integral JS numbers reach C as the exact double" test, uses identical harness conventions (tempDir, Promise.all pipe drain, toMatchObject on {results, stderr, exitCode}), and includes an echo_int control so the int32-tagged fast path is still asserted. The fixture .c updates are pure regeneration from FFI.h. PR description explains why this supersedes the earlier construction-time-wrapper approach (#33095).

[claude]

LGTM — the decode now mirrors JSVALUE_TO_DOUBLE and every generated JSVALUE_TO_INT32 caller in abi_type.rs (char/i8/u8/i16/u16/i32/u32) benefits. The inline nit about NaN/non-number UB is real but matches the file's existing JSVALUE_TO_PTR/_INT64/_UINT64 convention under TinyCC and isn't a regression, so not blocking.

Extended reasoning...

Overview

Fixes JSVALUE_TO_INT32 in src/runtime/ffi/FFI.h to handle double-encoded JSValues instead of blindly truncating the raw NaN-boxed asInt64. The two ffi.test.fixture.*.c files are regenerated copies of the same header (they embed FFI.h verbatim), and a new spawned-subprocess test in cc.test.ts covers i32/u32/i8/u16 JSCallback returns with double-encoded integers plus fractional/negative cases.

Correctness

The new branch mirrors the existing JSVALUE_TO_DOUBLE / JSVALUE_TO_PTR shape in the same file: check JSVALUE_IS_INT32, else subtract DoubleEncodeOffset and read asDouble. I verified via abi_type.rs:158-164 that every small-integer ABI type funnels through JSVALUE_TO_INT32 with an outer C cast, so the fix covers the whole class. The four internal FFI.h callers (lines 237/314/332/347) all pre-guard with JSVALUE_IS_INT32, so the added branch is a no-op for them — the PR body's claim checks out. Traced the u32 case: 3000000000.0 → (int64_t)3e9 → (int32_t) low-32-bits → (uint32_t) → 3000000000, and neg-fractional -5.7 → -5 via C truncate-toward-zero — both match the test expectations.

Security risks

None. This is a JSValue bit-pattern decode in a TinyCC-compiled trampoline; no untrusted-length arithmetic, no allocation, no pointer dereference introduced. The nit about C11 6.3.1.4 UB on NaN/out-of-range only fires when the user violates their own returns: 'i32' contract, is deterministic under TinyCC's cvttsd2si/fcvtzs lowering, and the pre-fix behavior on those inputs was already garbage — so no new attack surface.

Level of scrutiny

Low-medium. A 7-line change to one static inline function, following an established pattern two functions away in the same file. FFI is opt-in surface and the change strictly widens the set of correctly-decoded inputs without altering the int32-tagged fast path.

Other factors

Test follows the neighboring cc.test.ts conventions exactly (tempDir, bunEnv/bunExe, concurrent stdout/stderr/exited drain, toMatchObject on the combined object with stderr included but not asserted-empty, cb.close() cleanup). It pins double-encoding via x + 0.5 - 0.5 on a runtime arg so it fails-before regardless of JIT tier, and covers both the int32-tagged path (echoInt) and the new double path across four ABI widths. The one inline nit is explicitly non-blocking and applies equally to pre-existing code in the file.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/ffi/FFI.h`:
- Around line 276-280: Update JSVALUE_TO_INT32 in
src/runtime/ffi/FFI.h#L276-L280 to reject non-finite and values outside the
int32 range before narrowing, preserving the existing NaN fallback. Apply the
identical conversion guard in
test/js/bun/ffi/ffi.test.fixture.callback.c#L278-L280 and
test/js/bun/ffi/ffi.test.fixture.receiver.c#L278-L280 so both fixture copies
remain synchronized.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7060a4c8-876b-49b8-95f6-0f72a4cfcdb7

📥 Commits

Reviewing files that changed from the base of the PR and between 63f49f6 and 70fbcd8.

📒 Files selected for processing (3)

• src/runtime/ffi/FFI.h
• test/js/bun/ffi/ffi.test.fixture.callback.c
• test/js/bun/ffi/ffi.test.fixture.receiver.c

[claude]

LGTM — the NaN guard from the earlier nit is in (70fbcd8), and the rest is unchanged.

What was reviewed:

• JSVALUE_TO_INT32 decode mirrors JSVALUE_TO_DOUBLE; verified all internal callers in FFI.h and the abi_type.rs table (i8/u8/i16/u16/i32/u32 all funnel here via a cast prefix, so the int64_t intermediate for the u32 path is load-bearing).
• Confirmed the post-subtraction NaN check catches undefined/null/true/false/cell returns (their decoded bits all have exponent all-ones, non-zero mantissa) as the comment claims.
• Test follows the sibling subprocess-fixture pattern in the same describe block; covers int32-tagged, double-encoded, fractional ±, and u32-above-INT32_MAX.

Extended reasoning...

Overview

Eight-line change to JSVALUE_TO_INT32 in src/runtime/ffi/FFI.h (plus two mechanically-regenerated .fixture.c snapshots and one new test in cc.test.ts). The function previously returned val.asInt64 truncated, which is only correct for int32-tagged JSValues; it now checks the tag, and for double-encoded values subtracts DoubleEncodeOffset, guards NaN, and casts through int64_t. This is the exact pattern already used by JSVALUE_TO_DOUBLE a few lines down.

Security risks

None. FFI.h is compiled only by the in-process TinyCC for user-declared FFI trampolines; the change narrows the set of inputs that produce garbage rather than widening any attack surface. No new allocation, no new pointer arithmetic, no external data parsing.

Level of scrutiny

Moderate — this is a JSValue-encoding boundary, so it needs to be bit-exact against JSC's NaN-boxing scheme. I traced each case: int32-tagged (NumberTag mask) → low-32-bit truncation unchanged; double-encoded → offset subtraction matches JSVALUE_TO_DOUBLE/JSVALUE_TO_PTR; the u32 path ((uint32_t)JSVALUE_TO_INT32(...) per abi_type.rs:164) round-trips 3e9 correctly via the int64_t intermediate (float→int64 in-range, then implementation-defined int64→int32 truncation, then uint32 reinterpretation). Existing internal callers (JSVALUE_TO_PTR/_DOUBLE/_INT64/_UINT64) all pre-guard with JSVALUE_IS_INT32, so the new branch is dead for them.

Other factors

My earlier nit about NaN/non-number UB in the fallthrough was applied in 70fbcd8 and the thread is resolved; the remaining ±Infinity / |d|≥2⁶³ case is explicitly deferred to match the file's existing convention in _INT64/_UINT64/_PTR, which is reasonable scope. The test is placed alongside the three sibling encoding tests in the same describe, uses the same subprocess-fixture harness pattern (concurrent pipe drain, toMatchObject on {results, stderr, exitCode}, no stderr-empty assertion), and covers the variant matrix the review guide asks for (i32/u32/i8/u16, tagged vs double-encoded, ± fractional truncation, u32 > INT32_MAX). The duplicate-PR bot's hits were addressed by the author and don't overlap.