Repository navigation
fix(ffi): multiple long-standing FFI correctness bugs - #31449
Closed
ObscuritySRL wants to merge 13 commits into
Closed
ObscuritySRL wants to merge 13 commits into
ObscuritySRL wants to merge 13 commits into
Conversation
ObscuritySRL
marked this pull request as ready for review
May 27, 2026 00:12
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThis PR corrects FFI type conversion boundaries, refactors ABI validation, improves error handling, and adjusts type conversion logic. Integer encoding boundaries prevent overflow at power-of-2 limits; ABI validation is simplified in Rust; callbacks and memory access now validate inputs explicitly; type conversions are refined; and symbol metadata lookup in ChangesFFI type handling and validation fixes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jun 10, 2026
Closed
|
Hi! I'm experiencing what might be related to the "JSCallback constructor silently failed" bug mentioned in this PR. Issue
Minimal Reproductionimport { JSCallback, FFIType } from "bun:ffi";
const callback = new JSCallback(
{
args: [FFIType.ptr, FFIType.ptr, FFIType.i32],
returns: FFIType.i32,
},
(ctx, buffer, size) => {
console.log("Callback invoked");
return 0;
}
);
console.log("callback.ptr:", callback.ptr);
// Expected: A number (pointer address)
// Actual: undefinedOutput: Environment
Error When UsedWhen trying to pass this callback to a native function expecting The Bun FFI code checks How This Relates to PR #31449This seems directly related to two issues mentioned in this PR:
According to the official Bun FFI docs,
QuestionWill this PR fix the |
This was referenced Jun 29, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 1, 2026
### 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.
Collaborator
|
Heads up on an overlap: the Two differences worth folding in either way:
Also: the Happy to close #33340 if this one lands first. |
Rebased onto current main. The earlier double/BigInt argument fix from this branch is dropped: oven-sh#33122 already fixed that upstream (main's double wrapper now routes through Number(val)). JS (src/js/bun/ffi.ts): - cc(): read symbol specs from options.symbols[key], not options[key], so cstring returns become CString instances and argument wrappers (integer clamps, pointer auto-conversion) actually install. - int16_t arg wrapper: clamp to INT16_MAX (32767), not 32768 (which signed-overflowed to -32768 when the C trampoline cast to int16_t). - cstring/pointer arg wrapper: accept any object with a numeric .ptr (e.g. CString), matching the function wrapper's duck typing. - JSCallback constructor: throw the Error instance returned by nativeCallback() instead of destructuring it into ptr=undefined. - FFIBuilder: resolve numeric FFIType constants (e.g. FFIType.buffer = 20) as well as string labels, for both argument and return types. FFIType[n] reverse-maps a number to its label, so the old FFIType[params[i]] lookup threw "Unsupported type 20" for numeric constants. - Remove dead duplicate ffiWrappers entries (i64_fast/u64_fast/uint16_t early definitions overwritten by later ones). Rust/C: - FFI.h: INT64_TO_JSVALUE / UINT32_TO_JSVALUE use strict `< MAX_INT32`. MAX_INT32 is 2^31 (not INT32_MAX), so `<=` admitted 2^31 into the int32 encoding, where it wrapped to -2^31. - FFIObject.rs: addr_from_args returns a JS error on a negative byteOffset instead of panicking (read.u8(addr, -1) previously SIGABRT'd the process). - ffi_body.rs / host_fns.rs: the threadsafe-non-void-return guard now reads the local `threadsafe` (it read the not-yet-assigned struct field, so the guard never fired); drop the redundant ABIType::MAX filter that rejected FFIType.buffer (from_int already range-checks and accepts Buffer = 20). Tests: un-skip ping(cstr)/strlen(cstring) and add coverage for each fix in cc.test.ts and ffi.test.js. Fix makeValidCase to return a live handle (it returned undefined before its beforeAll ran; every caller was skipped until now). The FFI.h-derived ffi.test.fixture.*.c snapshots are regenerated. Built and tested on Windows x64 (debug): all new tests pass, and the same tests fail on system Bun 1.4.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AxxaB1U8TSsXg7RGyAtJhX
ObscuritySRL
force-pushed
the
claude/ffi-quick-fixes
branch
from
July 12, 2026 01:19
e0a6ce3 to
f52cfe9
Compare
Iteration from a 7-expert adversarial review of the FFI surface (31 raw
findings, 28 verified) plus independent analysis. All fixes reproduced and
covered by tests.
Memory safety / crashes reachable from public API with trivial input:
- toBuffer(ptr) without a finalizer installed MarkedArrayBuffer_deallocator and
mi_free'd the caller's foreign/interior pointer on GC (double-free / heap
corruption). Now a non-owning view via a no-op deallocator, matching
toArrayBuffer (a null deallocator asserts in debug / null-calls on GC).
- ptr()/toArrayBuffer()/toBuffer()/CString with a byteOffset of -Infinity or
exactly -(2^63) negated i64::MIN -> integer-overflow panic -> process abort
(panic=abort). Now uses unsigned_abs + a finiteness check and returns a clean
error. Fixes the sibling paths the prior addr_from_args fix missed.
- get_ptr_slice lacked ptr_'s max-addressable-memory bound, so a huge finite
byteOffset (2^63) reached the CString NUL-scan and segfaulted. Bound added.
- JSCallback accepted callable Proxies/InternalFunctions (isCallable) then
uncheckedDowncast<JSFunction>'d them (type confusion). Now dynamicDowncast +
reject; the Step::Failed error message no longer borrows freed heap memory.
Undefined behavior / platform divergence:
- i64_fast/u64_fast args passed out-of-range doubles straight to (int64_t)d /
(uint64_t)d, which is C UB and differs on x86 (indefinite) vs arm64
(saturating). FFI.h now clamps and maps NaN -> 0.
Security / DoS:
- cc({ symbols: { f: { args: new Array(0xFFFFFFFF) } } }) reserved ~16 GB up
front (reserve_exact on the JS array length) -> OOM abort. Reservation capped.
API / correctness:
- cc() string `flags` replaced the defaults, dropping -Wl,--export-all-symbols
so the compiled symbols never resolved. Now prepends the defaults and appends,
matching the array form and the documented example.
- napi_value args kept the default `val|0` wrapper (silent JSValue corruption);
DataView was rejected by the ptr/cstring/buffer wrappers though the types list
it; size_t was in the native table but missing from FFIType. Fixed.
- read.* read byteOffset with to_int32 (wraps at 2^31) while ptr()/toBuffer()/
toArrayBuffer() used to_int64; get_ptr_slice's non-number byteOffset handling
was inverted (silently ignored instead of erroring). Unified.
- Poison-pointer guard compared the same 32-bit constant twice; the 64-bit fill
was never caught. i64_fast/u64_fast wrappers allocated two BigInts per call.
- .d.ts: toBuffer/toArrayBuffer finalizer params; buffer/napi_env returns ->
never; bool coercion doc; DataView arg types; size_t.
Idioms: deleted dead ABIType::MAX and the dead c_param_type column /
param_typename / param_typename_label (generated C is byte-identical).
Tests: resurrected the dlopen round-trip suite (make compile-ffi-test was
removed -- compile the fixture with the system compiler at load time; fix the
hardcoded .dylib -> suffix); rewrote the broken threadsafe-callback test to
await the invocation and skip on Windows (matching cc.test.ts); removed the
"ffi print" test that overwrote git-tracked .c files and deleted those dead
fixtures; un-skipped three stale skips (string args coerce, missing arg -> 0,
syntax error throws); tightened a bare toThrow and the exact-empty-stderr
assertions; arch-selected the libc path; added regression tests for every fix.
Built and green on Windows x64 (debug): test/js/bun/ffi/** = 121 pass, 0 fail,
34 skip across 5 files. Release build, rust:check-all, benchmarks, and the
dead-code deletion of host_fns.rs are follow-ups tracked in GOAL_RESULTS.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
…C error sanitizer
host_fns.rs plus mod.rs's own Function/Step/Compiled structs and LIB_DIR_Z were a
second, dead copy of the FFI symbol-spec parsers and C-trampoline emitter — the
live implementation is ffi_body::Function (used by open/cc/linkSymbols/callback).
Verified dead: the mod.rs re-exports had no external callers and nothing
references crate::ffi::{Function,Step,Compiled} (ffi_body defines its own). Delete
the file, the mod/re-export, the duplicate structs + impls, LIB_DIR_Z, and the
now-unused imports (~470 lines).
Also extract strip_leading_nonprintable() so the two TinyCC error handlers
(handle_compilation_error, handle_tcc_error) share one copy of the
"skip leading non-printable garbage" logic instead of a copy-pasted loop with
bare 0x20/0x7f magic numbers.
No runtime change. Debug build clean (-D unused-imports enforced) and
`rust:check-all` green across linux/macOS/windows x64/arm64 (0 errors, 0
warnings). cc() error reporting verified unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
…pass
A second adversarial 7-expert pass over the committed FFI code surfaced 11 real
findings — several are missed siblings of the first pass's own fixes.
Memory safety:
- JSCallback compile failure leaked the FFICallbackFunctionWrapper (and a
Strong<GlobalObject> GC root pinning the whole realm) on every failure path
after wrapper creation — the null-check fix only covered a null wrapper. A
valid wrapper whose compilation then fails (e.g. `new JSCallback(fn, { args:
["buffer"] })`, whose callback-direction codegen emits invalid C) leaked
unboundedly. Fixed: (1) reject buffer/napi_env as callback ARG types up front;
(2) scopeguard the wrapper at the acquisition site, disarmed only when
Step::Compiled adopts it.
Crashes / UB (missed siblings of the byteOffset + float->int fixes):
- read.* fed a non-number byteOffset straight to to_int64() with no is_number
guard (the guard the ptr_/get_ptr_slice siblings have) -> JSC toInt64 ASSERT
abort (debug) / UB (release). Added the guard.
- get_ptr_slice bounded byteLength at MAX_ADDRESSABLE_MEMORY (2^56) but
ArrayBuffer::from_bytes does u32::try_from(len).expect(), so a byteLength
>= 2^32 panic-aborted. Tightened to u32::MAX.
- int8_t arg wrapper had no clamp (sibling of int16_t/uint8_t): (int8_t) wrapped
128 -> -128. Added the clamp.
- JSVALUE_TO_PTR had the same double->int UB fixed in INT64/UINT64; clamped.
Idioms:
- Removed dead EnumMapFormatter; derived compile()'s fallback flags from
DEFAULT_TCC_OPTIONS instead of a duplicated literal.
Tests:
- Strengthened the DataView test to actually validate byteOffset (offset-2
address == offset-0 address + 2); added the missing finalizer-callback GC test
(deallocator fires exactly once); fixed a stray expect(stderr).toBe("");
added regression tests for the int8_t clamp, the read.*/toArrayBuffer
crash-guards, and the callback buffer/napi_env rejection.
Debug build green; full test/js/bun/ffi/** = 126 pass / 0 fail / 34 skip (161
tests across 5 files).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
A third adversarial pass converged to 6 low-severity findings, all fixed:
- get_ptr_slice's u32::MAX byteLength ceiling only covered the explicit-length
branch; toArrayBuffer(ptr) with no length returned the full strlen, which
ArrayBuffer::from_bytes (u32::try_from(len).expect) would panic on for a >=4 GiB
NUL-free region. Applied the same ceiling to the NUL-scan branch.
- Removed two dead write-only fields: Step::Failed.allocated and
Compiled.js_context (never read; matches use `{ msg, .. }` / field access).
- Rewrote the stale ValueOrError consumer-audit comment that still described
toBuffer's already-fixed mi_free footgun as current behavior.
- Tests: added the missing wrapper-leak regression (loops JSCallbacks whose args
compile to invalid C so the compile_callback scopeguard's free-on-failure path
runs, then asserts a clean exit — the ASAN lane catches a regression); moved the
negative read.* byteOffset case into the spawned crash-guard fixture.
Debug build green; full test/js/bun/ffi/** = 127 pass / 0 fail / 34 skip (162
tests across 5 files).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
…rray
A fourth convergence pass found one real reachable crash the earlier passes
missed: an empty `source: []` built `Source::Files(Vec::new())`, so
`Source::first()`'s `&files[0]` indexed an empty Vec and panicked -> process
abort. Reachable from the public API (ffi.ts cc() only rejects a falsy source;
`[]` is truthy).
- Reject an empty source array in bun_ffi_cc with a clear JS error before compile.
- Defensively harden Source::first() to files.first()...unwrap_or(zstr!("")) so
no caller can index-panic.
- Fixed a stale mod.rs module-doc line (claimed the JSC offsets live there; they
live in ffi_body).
- Added a spawned regression test asserting cc({ source: [] }) throws, not aborts.
Debug build green; full test/js/bun/ffi/** = 128 pass / 0 fail / 34 skip (163
tests). Convergence series across review passes: 28 -> 11 -> 6 -> 2 findings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
The 5th convergence review found FFITypeStringToType omitted 8 labels the runtime
accepts (abi_type.rs ABI_TYPE_LABEL): i64_fast, u64_fast, c_int, c_uint, isize,
char*, void*, fn. So e.g. `dlopen(p, { f: { args: ["i64_fast"] } })` typed the arg
as `never` (uncallable in a typed codebase) even though the call works at runtime
— a missed sibling of the size_t/usize/callback additions in the same table.
Added the eight entries mapped to their canonical members. Types-only change.
(The bun-types integration test's harness is currently failing wholesale on this
Windows dev machine — 0/13 with and without this change, all on unrelated features
like bun:bundle/markdown/lib-config — so it was verified by inspection against the
runtime table; the change is purely additive to a lookup interface.)
Convergence series across review passes: 28 -> 11 -> 6 -> 2 -> 1.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
… hermetic The 6th convergence pass found a real feature breakage: a symbol declared with `returns: "napi_value"` and no napi_env/napi_value argument failed to bind. The generated forward wrapper opens a NapiHandleScope (referencing Bun__thisFFIModuleNapiEnv) whenever needs_handle_scope() is true — which includes a napi_value return — but the env symbol was only added when needs_napi_env() (args only). So the wrapper referenced an unresolved symbol, relocate() failed, and the symbol never bound across dlopen()/cc()/linkSymbols(). Fixed by gating the env on needs_handle_scope() (a strict superset of needs_napi_env, so nothing else regresses) in both make_napi_env_if_needed (the dlopen/linkSymbols batch env) and the CompileC shared-state loop (cc()). Added a cc() regression test that binds a napi_value-returning symbol with no napi args. Also fixed addr32.test.ts to build its .so into a tempDir (auto-removed) instead of writing libaddr32.so into the git-tracked test directory with no cleanup. Debug build green; full test/js/bun/ffi/** = 129 pass / 0 fail / 34 skip (164 tests). Convergence series: 28 -> 11 -> 6 -> 2 -> 1 -> 2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
…p dead vm The 7th convergence pass found no crash/UB/security/correctness defect — only three quality nits, all fixed here: - get_ptr_slice's non-finite error said "ptr must be a finite number." when it is the byteOffset that is bad (the ptr was already validated). Corrected to "byteOffset must be a finite number", matching ptr_(). - .d.ts FFITypeStringToType mapped "function"/"callback" to FFIType.pointer while the runtime (and the just-added "fn") map them to FFIType.function — so a JSCallback arg was a spurious TS error and a TypedArray type-checked but threw at runtime. Mapped both to FFIType.function. - Removed a dead `let vm = jsc::VirtualMachineRef::get();` (+ its `let _ = vm;` discard) in FFI::open. Debug build green; full test/js/bun/ffi/** = 129 pass / 0 fail / 34 skip (164 tests). Convergence: 28 -> 11 -> 6 -> 2 -> 1 -> 2 -> 3 (all cosmetic, 0 real bugs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
CompileC::compile obtained a TCC::State handle (an opaque FFI object with no
Drop) but installed no scopeguard over it, unlike the sibling Function::compile.
Every error return — invalid C, an unknown exported symbol, a bad include or
library path, a relocate failure — leaked a full TinyCC context (section
buffers, symbol tables, code cache). The path is user-reachable and repeatable,
so RSS grew unbounded across repeated failed cc({...}) calls (~216 KB each).
Wrap the state in a scopeguard that destroys it on any early return, disarmed
via ScopeGuard::into_inner on the success path where ownership transfers to the
caller — mirroring the existing guard in Function::compile.
Regression test loops 300 failed compiles and asserts bounded RSS + clean exit;
reverting only the scopeguard makes it grow 64.8 MB (vs 2.5 MB fixed), and the
ASAN lane independently flags the leak.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
…ites (UAF)
When a per-symbol wrapper fails to compile, the reason is stored in
Step::Failed { msg }, where msg is a heap Box<[u8]> owned by the Function.
cc(), dlopen(), and linkSymbols() converted it with
ZigString::init(msg).to_error_instance(global), which (via getErrorInstance ->
toString -> StringImpl::createWithoutCopying) makes the JS error's message
StringImpl borrow msg's bytes with no copy. The native call then returns, the
local symbols map drops, the Function drops, and msg is freed — but user JS
reads error.message after the call returns, dereferencing freed heap: a
use-after-free (garbage bytes, or an ASAN abort).
An earlier iteration fixed this in the JSCallback path but never propagated the
fix to these three siblings. Extract one failed_step_error() helper that copies
msg via create_error_instance(format_args!) and route all four sites (the three
siblings plus the callback path) through it, so the borrow-vs-copy contract has
a single owner and a future site can't reintroduce the class.
Reachable from script: args:["void"] compiles to `void arg0` (invalid C) with a
resolved symbol, landing in Step::Failed. The new spawned regression test drives
cc()/dlopen()/linkSymbols() that way and asserts each error.message is printable
ASCII — GARBAGE on the unfixed build, OK on the fixed build.
Also scope ffi.test.js's "Failed to dlopen the ffi test fixture" log to the
fixture itself, so the intentional dlopen("nonexistent") test stops logging it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
ffi.ts saturation-clamps four 8/16-bit arg types before the no-cast trampoline truncates the low byte(s): uint8_t/int8_t/int16_t/uint16_t. Out-of-range tests existed for int8_t and int16_t but not the two unsigned types, so reverting either uint clamp to a bare `val|0` would leave CI green while reintroducing 256->0 / 65536->0 wraparound. Add uint8_t and uint16_t arg-clamping tests mirroring the existing int ones. Verified load-bearing: reverting the clamps makes exactly these two tests fail (identity_uint16(65536) returns 0), int8_t/int16_t unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
Addresses the review feedback on oven-sh#31449 and folds in the overlapping oven-sh#33340. - MAX_INT32 held 2^31 (INT32_MAX + 1) — a mis-named constant that forced every comparison to a strict `<` workaround. Correct it to INT32_MAX (2147483647) and add a distinct MIN_INT32, so the int32-range checks read naturally with inclusive bounds. Behavior at every boundary is unchanged (verified by the existing 2^31 / -2^31 return tests). - UINT64_TO_JSVALUE compared against MAX_INT52 with a strict `<`, so a u64_fast return of exactly Number.MAX_SAFE_INTEGER arrived as a BigInt while the same value from i64_fast arrived as a Number. Use `<=` so both agree — it is exactly representable as a double. Tests: extend the cc.test.ts boundary describe to cover the u32/u64 int32 boundary and the u64_fast/i64_fast MAX_SAFE_INTEGER agreement, and add ASan-covered coverage through dlopen (the cc() variants are ASan-skipped) via new boundary functions in the ffi-test.c fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01279WZmDr9EtyQ1FG97AioG
Author
|
This one's ready for review whenever someone has a moment. Since I opened it I did a thorough pass over the whole FFI surface and folded the results into this branch, so it now covers quite a bit more than the original description — I've rewritten the PR body to match. Highlights:
I also folded in the robobun note about the overlap with #33340 — the CI shows as blocked pending a maintainer trigger — could someone kick off the build when convenient? Happy to rebase, split it up, or adjust anything to make review easier. |
This was referenced Jul 18, 2026
Collaborator
|
Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @ObscuritySRL 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.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens Bun's FFI subsystem (
bun:ffi—dlopen/cc/linkSymbols/CFunction/JSCallback/ptr/read.*/toBuffer/toArrayBuffer/CString). It began as a batch of long-standing correctness fixes, then the entire FFI surface was run through repeated rounds of independent adversarial review until a fresh full pass found nothing. Net −558 lines (+1157 / −1715 — a large amount of dead code removed).Every behavioral change ships a regression test that fails on unmodified Bun and passes here. Debug + release builds,
bun run rust:check-all(all target triples), and the fulltest/js/bun/ffi/**suite are green on both debug and release (133 pass / 34 skip / 1 todo / 0 fail). Hot-path benchmarks (arg marshalling, return encoding, buffer creation,read.*) show no regression.Memory-safety fixes (the most serious)
cc(),dlopen(), andlinkSymbols()built theirStep::Failederror viaZigString::init(msg).to_error_instance(...), which (throughgetErrorInstance→toString→StringImpl::createWithoutCopying) makes the JS error's.messageborrow a heapBox<[u8]>owned by aFunctionthat is freed before user JS reads it. Reachable viaargs:["void"](invalid C on a resolved symbol →Step::Failed); on the unfixed builderror.messagewas 47 bytes of freed/poisoned memory, and an ASAN build aborts. Fixed by routing all four sites (including the callback path, which had already been fixed in isolation) through one sharedfailed_step_errorhelper that copies the message.CompileC::compileobtained aTCC::State(noDrop) but installed no scopeguard, unlike its siblingFunction::compile. Every failedcc()compile — invalid C, an unknown symbol, a bad include path — leaked a full TinyCC context (~216 KB). Now freed on every path; verified as 64.8 MB → 2.5 MB RSS growth over 300 failed compiles.Crash / DoS fixes (uncatchable process aborts reachable from one public call)
ptr(a, -Infinity)/-(2**63)/NaNbyteOffset → integer-overflow panic (-i64::MIN) → abort. Now a cleanTypeError: byteOffset must be a finite number.read.u8(addr, -1)/ non-number byteOffset →JSValue::toInt64ASSERT abort. Now guarded (undefined→0, non-number→error).toArrayBuffer(ptr, 0, 2**33)(byteLength ≥ 2³²) →u32::try_from(len).expect()panic. Now bounded tou32::MAXon both the explicit-length and NUL-scan paths.cc({ source: [] })(empty source array) → empty-Vecindex panic. Now rejected with a clear error.new Array(0xFFFFFFFF)asargs→ ~16 GBreserve_exact→ OOM abort. Reservation hint now capped.toBuffer(ptr(typedArray))→ JSC installedmi_freeas the deallocator and freed foreign/interior memory on GC (double-free / allocator mismatch). Now a non-owning view, matchingtoArrayBuffer.Undefined behavior / platform divergence
i64_fast/u64_fast/ pointer args inFFI.hwas C UB and platform-divergent (x86 →INT64_MIN, arm64fcvtzs→ saturates). Now clamped (NaN→0, saturate to range) — defined and identical on every target.uint8_t/int8_t/int16_t/uint16_t) saturate instead of letting the C cast wrap (256→0, 128→−128, 32768→−32768, 65536→0).INT64/UINT64/UINT32_TO_JSVALUE):MAX_INT32was mis-named — it held 2³¹, notINT32_MAX— so a 64-bit function returning 2³¹ wrapped to −2³¹ in JS. The constant is corrected toINT32_MAX(with a distinctMIN_INT32). Separately,u64_fastcompared againstMAX_INT52with a strict<, so exactlyNumber.MAX_SAFE_INTEGERcame back as aBigIntwhilei64_fastreturned aNumber;u64_fastnow uses<=so both agree. This reconciles with ffi: u32 and i64_fast returns of 2 ** 31 arrive in JS as -2147483648 #33340 (a fuzzing-driven fix of the same block), which can be closed once this lands.API correctness
cc()reads symbol specs fromoptions.symbols[key], notoptions[key]— so cstring returns actually becomeCStringinstances and the argument wrappers (integer clamps, pointer auto-conversion) actually install.JSCallbackrejects non-JSFunctioncallables (a callableProxy/InternalFunctionpreviously hituncheckedDowncast<JSFunction>— type confusion) and throws the returnedErrorinstead of destructuring it intoptr = undefined.napi_valueargs are no longer coerced throughval|0;returns: "napi_value"with no napi args now binds (the handle-scope env symbol was only added for napi args);DataViewis accepted as aptr/bufferarg;size_tand several string type labels (i64_fast,u64_fast,c_int,c_uint,isize,char*,void*,fn) were missing from the JSFFIType/.d.ts.cc()stringflagsprepend the default flags instead of replacing them (a stringflagssilently dropped-Wl,--export-all-symbols, so symbols never resolved).Dead code removed
src/runtime/ffi/host_fns.rs(469 lines) — an entire dead second copy of the symbol-spec parser and C-trampoline emitter (the live one is onffi_body::Function).test/js/bun/ffi/ffi.test.fixture.*.csnapshots (~790 lines) that nothing asserted against and that were rewritten on every run.mod.rs's deadFunction/Step/Compiled/LIB_DIR_Z,ABIType::MAX,param_typename/param_typename_label, thec_param_typetable column,EnumMapFormatter, and other orphaned fields.Tests
A regression test for every fix, most spawned as subprocesses so a crash regression surfaces as a child abort (not a runner abort) and each verified to fail on unmodified Bun. The previously-dead
dlopenround-trip suite — gated on a file produced by a Makefile target deleted in #21863, so it never ran — is resurrected by compiling the fixture with the system compiler at load time.The 34 skips + 1 todo are all platform/build gates, never silenced failures: TinyCC is unavailable on Windows-ARM64 (
isFFIUnavailable); a few subprocess-timing tests are ASAN-incompatible; the foreign-thread threadsafe-callback tests and NaN-purification test are Windows-gated (they run on POSIX CI); and the>63 symbolsand int32-address tests are Linux/glibc-only.Testing & caveats
Built and validated on Windows x64 (debug + release). Cross-platform is covered by
bun run rust:check-all(all six target triples type-check); full test execution on Linux/macOS and arm64 is left to CI. DOMJIT fast paths forptr/read.*were evaluated and deliberately left as a WONTFIX (a separate, higher-risk perf project; disabled system-wide in #14005 and the fast-path bodies don't exist insrc/).🤖 Generated with Claude Code