bun:ffi: coerce JSCallback return values to the declared return type - #33095
bun:ffi: coerce JSCallback return values to the declared return type#33095robobun wants to merge 1 commit into
Conversation
WalkthroughAdds ChangesFFI size_t and JSCallback return coercion
Docs reformatting
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 4:16 AM PT - Jul 1st, 2026
❌ @robobun, your commit 950d956 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33095That installs a local version of the PR into your bun-33095 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
All my earlier feedback has been addressed and the bug hunt found nothing new — deferring only because this touches the FFI trampoline path and CODEOWNER'd packages/bun-types/, so it should get a human sign-off.
Extended reasoning...
Overview
The PR wraps JSCallback so its return value is coerced through the existing ffiWrappers table before the native trampoline reads it, fixing cases where raw JSValue encodings (heap-pointer bits, tag values, IEEE bit patterns) leaked to C. It also rewrites ffiWrappers[FFIType.double] to use proper ToNumber semantics (the old val + (0.00 - 0.00) string-concatenated for non-numbers), adds size_t to both the runtime FFIType map and the .d.ts FFITypeStringToType, and adds a ~27-case subprocess test plus a bun-types fixture line. The two docs/*.mdx diffs are pure autofix.ci whitespace/table-realignment with no content change.
Security risks
The change is security-positive: it closes an ASLR-relevant heap-pointer leak to native code. The new FFICallbackReturnWrapper uses new Function(...) for codegen, but that mirrors the long-standing FFIBuilder pattern in the same file and the interpolated pieces (ffiWrappers[returnTypeId], generated p0,p1,... param names) are all module-internal constants — no user-controlled strings reach the generated source. I don't see new injection, auth, or data-exposure surface.
Level of scrutiny
Moderate-to-high. bun:ffi sits at the JS↔native boundary where coercion mistakes mean wrong values handed to C, and packages/bun-types/ffi.d.ts is CODEOWNER'd. The f64 wrapper change also affects the existing argument direction for dlopen/CFunction (e.g., undefined as an f64 arg now becomes NaN via +undefined rather than 0 via the old !val branch) — that's the more correct ToNumber behavior and is covered by the new f64_arg_* test cases, but it is a user-visible semantics change worth a human eye.
Other factors
All three of my prior inline comments (missing size_t in the JS map, comment-length style nit, missing size_t in the .d.ts) were addressed in 42c7b78 and 540d32d and are resolved. The CI failures reported by robobun (AsyncLocalStorage-tracking.test.ts segfault on macOS x64, test-net-connect-memleak.js) are unrelated to FFI. The new test is thorough and self-contained (no external library needed). Given the CODEOWNER coverage on packages/bun-types/ and the FFI-boundary nature of the core change, I'm deferring rather than approving.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/bun-types/ffi.d.ts`:
- Line 427: The new string mapping for "size_t" is in place, but the public
FFIType enum in ffi.d.ts still does not declare the matching size_t member, so
the type surface is out of sync with the runtime export from FFIType in
src/js/bun/ffi.ts. Update the FFIType enum to include the size_t alias alongside
the existing members, keeping it consistent with the "size_t" entry in the FFI
type map so both FFIType.size_t and "size_t" are accepted.
In `@src/js/bun/ffi.ts`:
- Around line 338-340: The return-type lookup in FFI wrapper logic only handles
string keys, so enum-based callers passing numeric FFIType values are not
resolved correctly. Update the return type resolution in the ffi.ts path around
the FFIType lookup so it accepts both string names and numeric enum values
before deciding whether to skip coercion; use the existing returnTypeId check in
the wrapper that handles the raw JSValue/void cases. Ensure the branch that
builds the return coercion path recognizes FFIType.i32, FFIType.size_t, and
similar numeric inputs, not just options?.returns string forms.
🪄 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: 95eff890-332c-4c1d-95ec-50cd2ab98d5a
📒 Files selected for processing (6)
docs/guides/util/base64.mdxdocs/runtime/web-apis.mdxpackages/bun-types/ffi.d.tssrc/js/bun/ffi.tstest/integration/bun-types/fixture/ffi.tstest/js/bun/ffi/ffi.test.js
|
CI status, for whoever picks this up: the change is complete and none of the red is related to it. The
Earlier builds of this PR also hit, once each: I have already used one |
### What
`f64`/`double` arguments are silently corrupted before the native
function sees them. With a C callee that reports what it actually
received:
```js
const lib = dlopen(path, {
isnan_f64: { args: ["f64"], returns: "i32" }, // x != x
signbit_f64: { args: ["f64"], returns: "i32" }, // raw sign bit of x
echo_f64: { args: ["f64"], returns: "f64" },
});
lib.symbols.isnan_f64(NaN); // 0: C received 0.0
lib.symbols.signbit_f64(-0.0); // 0: C received +0.0
lib.symbols.echo_f64(-5n); // 5: C received the absolute value
lib.symbols.echo_f64(2n ** 1024n); // TypeError: Cannot mix BigInt and other types
lib.symbols.echo_f64(-(2n ** 1024n)) // +Infinity: wrong sign, no error
```
`f32` arguments and the `f64` return path are correct; the bug is
specific to the `f64` argument wrapper. These corrupt silently, and NaN,
signed zero, and negative integers are exactly the values numeric C
libraries assign meaning to.
### Cause
`ffiWrappers[FFIType.double]` in `src/js/bun/ffi.ts`:
```js
if (typeof val === "bigint") {
if (val.valueOf() < BigInt(Number.MAX_VALUE)) {
return Math.abs(Number(val).valueOf()) + (0.00 - 0.00);
}
}
if (!val) {
return 0 + (0.00 - 0.00);
}
return val + (0.00 - 0.00);
```
- `!val` is true for `NaN` and `-0.0`, so both become `+0.0`.
- The BigInt branch looks like it meant to range-check `|val| <
Number.MAX_VALUE` and return `Number(val)`, but the `Math.abs` ended up
on the return value, so every negative BigInt loses its sign.
- A BigInt at or above `Number.MAX_VALUE` falls through to `val + (0.00
- 0.00)`, which throws `TypeError: Cannot mix BigInt and other types`;
one at or below `-Number.MAX_VALUE` passes the `<` check and comes out
as `+Infinity`.
- A string argument survives the wrapper as a string (`"2.5" + 0` is
`"2.50"`), so the compiled stub reinterprets a `JSString` pointer as a
double.
The native decoder (`JSVALUE_TO_DOUBLE` in `src/runtime/ffi/FFI.h`)
already handles every JS number, including NaN, -0.0, and int32-tagged
values (covered by the existing "integral JS numbers reach C as the
exact double" test). The wrapper's only job is to guarantee the stub
receives a plain number.
### Fix
```js
if (typeof val === "number") {
return val;
}
return Number(val);
```
Numbers pass through bit-exact. Everything else is converted with
`Number()`: BigInt keeps its sign (out-of-range values become
+/-Infinity instead of throwing, matching `Number(bigint)` everywhere
else), strings and objects go through ToNumber, and `undefined` becomes
`NaN` instead of `0.0` (matching the `f32` wrapper, which is
`Math.fround(val)`).
### Test
Added to the existing `double <-> JSValue conversions` group in
`test/js/bun/ffi/cc.test.ts`. The fixture tinycc-compiles C observers
(`x != x`, the raw sign bit via a union, an echo) and calls them through
`CFunction`, which uses the same `FFIBuilder`/`ffiWrappers` argument
path as `dlopen` (the `dlopen` suite in `ffi.test.js` is gated on a
prebuilt library that no longer gets built, and `cc()` symbols currently
skip the argument wrappers entirely, so calling them directly would not
cover this). Assertions are on the values C reports, so a JS round trip
cannot mask an argument bug.
On the unfixed build, the one assertion reports every corruption:
```
nan_isnan: expected 1, got 0
negative_zero_signbit: expected 1, got 0
negative_bigint: expected "-5", got "5"
huge_bigint: expected Infinity, got a thrown TypeError
negative_huge_bigint: expected "-Infinity", got "Infinity"
string: expected "2.5", got "NaN" (JSString pointer read as a double)
undefined_arg: expected "NaN", got "0"
```
With the fix, `bun bd test test/js/bun/ffi/` passes (18 pass, 0 fail,
ASAN debug build).
### Out of scope, and overlap with other open PRs
The integer argument wrappers in the same table have their own problems
(saturation vs. two's-complement wrapping is inconsistent across widths,
and `int16_t`'s clamp bound of `32768` does not fit in int16). That is a
behavior decision beyond this bug, and #31449 already proposes changes
there.
- #31449 also fixes the BigInt sign half of this wrapper, but keeps the
`!val` branch, so NaN and -0.0 arguments are still corrupted with that
patch applied.
- #33095 fixes a different bug (JSCallback return values were never
coerced). Callback returns reuse the `ffiWrappers` table, so that PR
rewrites this same `FFIType.double` entry to an equivalent conversion.
The src hunk overlaps; the subjects and most of the coverage do not
(that PR asserts what C receives from callback returns, this one asserts
what a symbol receives as an `f64` argument, including the sign bit of
`-0.0` and out-of-range BigInts). Whichever lands second is a one-hunk
rebase, and this test applies unchanged either way.
The generated trampoline decodes the callback's return value with the raw JSValue macros in FFI.h (JSVALUE_TO_INT32 is `(int32_t)val.asInt64`), which assume the value already has the declared C type's representation. dlopen arguments get that guarantee from ffiWrappers via FFIBuilder, but nothing coerced a JSCallback's return value, so any JS value that was not already an int32 reached native code as its raw NaN-boxed encoding: objects and strings leaked the low 32 bits of their JSCell heap pointer, undefined became 10, true became 7, a double became its truncated IEEE bits, and returns:"bool" treated every value except `true` as false. Route the return value through the same per-type ffiWrappers coercion the argument direction uses. "size_t" is also added to the JS FFIType map and the bun-types string map: it is the one label the native ABIType parser accepts that the JS side did not, so it would have bypassed the coercion.
6d2b92b to
950d956
Compare
## 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
```ts
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.
<!-- robobun:evidence:begin -->
---
**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:evidence:end -->
|
Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @robobun for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
|
Verified: #35246 covers this completely. I ran my original probe against current The one semantic difference from this PR is that strings as number/pointer returns now throw No piece of this needs to survive separately. The only leftover is that |
2 participants
What
A
JSCallbackwith an integer,ptr,bool, or float return type hands the native caller the raw JSValue encoding whenever the JS return value is not already stored in that exact representation.returns: "ptr"returning an object gives0xffffffffffffffff,returns: "bool"returning1givesfalse, andreturns: "i64"returning a string tripsASSERTION FAILED: value.isHeapBigInt() || value.isNumber()in a debug build. Beyond being wrong values fed into native logic, the integer cases leak heap pointer material (ASLR-relevant) to whatever native code consumes the callback's result.Cause
The generated trampoline converts the callback's return value with the raw decoding macros in
src/runtime/ffi/FFI.h, which only reinterpret bits:Those macros are written for JSValues that already have the target representation. The argument direction gets that guarantee from the
ffiWrapperscoercion table, applied in JS byFFIBuilderbefore the native stub runs. Nothing applied those wrappers to aJSCallback's return value, so the raw encoding went straight into(int32_t)JSVALUE_TO_INT32(...).Fix
src/js/bun/ffi.ts:JSCallbacknow wraps the user's function so its return value goes through the same per-typeffiWrapperscoercion the argument direction uses, restoring the invariant the C macros assume.returns: "void"(nothing to coerce) andreturns: "napi_value"(the raw JSValue is the return value) are left alone, as are non-function callbacks so the existing native error path is unchanged.This gives the return direction the same semantics as passing the same value as an argument of that type:
ToInt32for the int32 family,!!forbool,BigIntfori64/u64, and a thrownTypeErrorfor values that cannot become aptr."size_t"is also added to the JSFFITypemap and toFFITypeStringToTypeinbun-types: it is the one type label the nativeABITypeparser accepts that the JS map was missing, so it resolved toundefinedand would have bypassed the coercion (and already bypassedFFIBuilderfor arguments).Verification
New test in
test/js/bun/ffi/ffi.test.js("JSCallback return values are coerced to the declared return type"). It invokes each JSCallback's trampoline through aCFunctionwith the same signature, so the JS side observes exactly what native code received; no external library orcc()is needed. 24 cases acrossi32/u32/u8/i16/bool/f64/f32/ptr/i64/u64/size_t, plus a bun-types fixture assertion forreturns: "size_t".On the unfixed build all of the cases above fail with the leaked encodings shown; with the fix they all pass. The existing
callbacks > fn(T) Tround-trip matrix inffi.test.jsand theJSCallback/ccsuites incc.test.tsstill pass.Rebase note
An earlier revision of this PR also rewrote
ffiWrappers[FFIType.double], because the callback return path reuses it and itsval + (0.00 - 0.00)body string-concatenated for non-number inputs. #33122 landed an equivalent fix for that wrapper on main first, so this branch is rebased onto it and no longer touches the double wrapper; thef64return cases here are covered by main's version.Related
cc()never applyingFFIBuilder(it indexesoptions[key]instead ofoptions.symbols[key]), which I hit while verifying this; that is intentionally not duplicated here.