bun:ffi: propagate exceptions from viewSource callback descriptor getters by robobun · Pull Request #33200 · oven-sh/bun

robobun · 2026-07-01T19:34:54Z

Two bun:ffi entry points convert a pending JS exception from the symbol descriptor's getters into a non-empty "Out of memory" error value, leaving the real exception set on the VM. A host function must not return a non-empty value while an exception is pending, so this trips the assertion:

Assertion failed: Native function returned a non-zero JSValue while an exception is pending
panic: host fn return/exception state mismatch

Both repros:

Bun.FFI.viewSource({ get args() { throw new Error("boom") } }, true);
new JSCallback(() => {}, { get args() { throw new Error("boom") } });

Cause: print_callback and FFI::callback both handled the error arm of generate_symbol_for_function (a JsResult<Option<JSValue>> that reads the descriptor's args, threadsafe, returns, and ptr properties) with .unwrap_or_else(|_| Some(<"Out of memory" error instance>)). Err(JsError::Thrown) means a JS exception is already pending on the VM, so substituting a non-empty return value violates the host function contract (the return value must be empty iff an exception is pending) and also mislabels the user's exception as "Out of memory".

Fix: ?-propagate at both sites so the getter's exception is thrown to JS. print_callback is widened to JsResult<JSValue> (its only caller, print, already returns that), which also drops the create_utf8_for_js(...).unwrap_or(JSValue::ZERO) on its return path. print()'s own generate_symbols(...).unwrap_or(Some(JSValue::ZERO)) is switched to ? as well; it was correct for Err(Thrown) but not for a future Err(OutOfMemory).

Not changed: open() and link_symbols() use the same unwrap_or(Some(JSValue::ZERO)). That form is correct there: JSValue::ZERO with the exception left pending is exactly how a host function signals it, and I verified dlopen and linkSymbols with a throwing args getter already propagate the getter's error on an unfixed build. Widening those two to JsResult would ripple through every bare JSValue return in both for an OutOfMemory variant no callee can currently produce.

Test: test/js/bun/ffi/ffi-viewSource-non-object.test.ts covers viewSource(descriptor, true) and new JSCallback(fn, descriptor) for each of the four getter-readable properties, plus the non-callback viewSource path. Before the fix the file aborts with the panic above; after, the getter's error propagates as a normal throw.

Found while auditing for the bug class from #28786 (returning the "exception pending" sentinel without an exception actually pending). This is the same assertion, in the opposite direction.

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi-viewSource-non-object.test.ts

robobun · 2026-07-01T19:35:04Z

^{Updated 5:08 PM PT - Jul 19th, 2026}

❌ @robobun, your commit 984b431 has 1 failures in Build #75852 (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

🧪 To try this PR locally:

bunx bun-pr 33200

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

bun-33200 --bun

coderabbitai · 2026-07-01T19:39:09Z

Walkthrough

FFI::print_callback's return type changes from JSValue to JsResult, using error propagation via ? instead of manual fallback values. FFI::print's callback branch calls it directly. A new test verifies descriptor getter exceptions propagate correctly in Bun.FFI.viewSource.

Changes

FFI print_callback error handling

Layer / File(s)	Summary
print_callback JsResult propagation `src/runtime/ffi/ffi_body.rs`	`print_callback` now returns `JsResult<JSValue>`, wrapping invalid inputs in `Ok(...)`, propagating symbol-generation failures via `?`, returning error instances instead of a `JSValue::ZERO` fallback, and `print`'s callback branch calls it directly without extra `Ok(...)` wrapping.
Descriptor getter exception test `test/js/bun/ffi/ffi-viewSource-non-object.test.ts`	Adds a `test.each` matrix over `args`, `threadsafe`, and `returns` descriptor getters that throw, asserting `Bun.FFI.viewSource` propagates the same error message, with comments documenting the prior swallowed-exception behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Print as FFI::print
  participant PrintCallback as FFI::print_callback
  participant SourceGen as print_callback_source_code

  Print->>PrintCallback: call for callback case
  PrintCallback->>SourceGen: generate source code
  SourceGen-->>PrintCallback: JsResult (Ok or Err)
  PrintCallback-->>Print: JsResult<JSValue> propagated via ?

Estimated code review effort: Medium

Suggested labels: bun:ffi

Suggested reviewers: Jarred-Sumner

Poem:
A rabbit hopped through callback code,
Where errors once got swallowed whole,
Now JsResult shows the road,
Each thrown exception plays its role.
Tests confirm the fix takes hold — 🐰✨

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: propagating FFI viewSource callback descriptor getter exceptions.
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.
Description check	✅ Passed	The PR description is detailed and covers the change, rationale, and verification, even though it does not use the repository’s exact headings.

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

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_body.rs`:
- Around line 1343-1352: The error handling in callback(), print(), open(), and
link_symbols() is still flattening Err results from
generate_symbol_for_function() and generate_symbols() into synthetic JSValue
values, which drops pending JS exceptions from throwing getters. Update these
call sites to propagate the JsResult with ? the same way print_callback() does,
preserving the original exception instead of converting it to Out of memory or
JSValue::ZERO.

🪄 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: 244780e7-274b-4cd6-b0d0-aa105eb0a572

📥 Commits

Reviewing files that changed from the base of the PR and between eba370b and 86f5e02.

📒 Files selected for processing (2)

src/runtime/ffi/ffi_body.rs
test/js/bun/ffi/ffi-viewSource-non-object.test.ts

claude

LGTM — all prior feedback addressed (callback() sibling, print() non-callback branch, and the ptr getter in both test matrices).

Extended reasoning...

Overview

This PR fixes a debug-assertion panic in bun:ffi where viewSource(descriptor, true) and new JSCallback(fn, descriptor) would trip "host fn return/exception state mismatch" if a descriptor property getter threw. The fix replaces .unwrap_or_else(|_| Some(<error instance>)) with ?-propagation at three call sites in src/runtime/ffi/ffi_body.rs (callback(), print_callback(), print()), widening print_callback's return type to JsResult<JSValue>. Tests in test/js/bun/ffi/ffi-viewSource-non-object.test.ts cover all four getter-readable descriptor properties (args, threadsafe, returns, ptr) across both entry points plus the non-callback viewSource path.

Security risks

None. This is error-propagation plumbing on a debug-only introspection API (viewSource) and the JSCallback constructor's argument-validation path. No new attack surface, no changes to trust boundaries.

Level of scrutiny

Low-to-moderate. The diff is ~30 lines of mechanical change: three .unwrap_or_else/.unwrap_or → ? swaps and the Ok(...) wrapping needed to widen one function's return type. The pattern being applied is the codebase's standard idiom for JS exception propagation. The only caller of print_callback (print) already returns JsResult, so the widening is contained.

Other factors

I reviewed this PR twice previously. The first round flagged the byte-identical sibling in FFI::callback — fixed in 124eacd along with print()'s non-callback branch. The second round flagged the missing ptr entry in the test matrices — fixed in a0fe876. The two remaining discussion points (the open()/link_symbols() sites and the pre-existing function.threadsafe dead-check) were deliberately and reasonably scoped out by the author with clear justification in the PR body and thread replies: the former is correct for the user-reachable Thrown case and widening would ripple ~25 return sites for an unreachable OOM arm; the latter is a behavioral change that belongs in its own PR. All inline threads are resolved. The bug-hunting system found no issues on the current revision. Test coverage is thorough and asserts the specific error message, not just "throws something".

…ad of returning them (#34396) ## What does this PR do? `viewSource` from `bun:ffi` returned a `TypeError` instead of throwing it when the descriptor was invalid: ```js import { viewSource } from "bun:ffi"; const result = viewSource({ myFunc: 42 }); result instanceof TypeError; // true, returned not thrown result.message; // 'Expected an object for key "myFunc"' ``` `JSCallback` has the same bug: its constructor destructured the returned error, so validation failures produced a useless instance instead of throwing: ```js import { JSCallback } from "bun:ffi"; const cb = new JSCallback(() => {}, 42); // options must be an object cb.ptr; // undefined, no error surfaced ``` Both happen for every validation failure the native implementation reports: non-object descriptors, unknown FFI type names, a non-object options argument, and (for `JSCallback`) a non-callable callback. ### Cause The native host functions on `Bun.FFI` return an `Error` instance as their value when validation fails; the `bun:ffi` module is expected to turn that into a throw. `dlopen`, `cc` and `linkSymbols` already do this with `if (Error.isError(result)) throw result;`, but `viewSource` was exported as a direct alias of the native function and `JSCallback` destructured the native result without checking it. ### Fix Wrap `viewSource` and guard the `JSCallback` constructor the same way as the three existing siblings. The declared return type of `viewSource` in `packages/bun-types/ffi.d.ts` is `string[] | string`, which this now matches. ### Why this is correct Matching `dlopen`/`cc`/`linkSymbols` keeps the `bun:ffi` module internally consistent, and it matches the typed contract. The low-level `Bun.FFI` object is left unchanged (it is not part of the typed public surface and `Bun.FFI.dlopen`/`Bun.FFI.linkSymbols` also return errors). ### How did you verify your code works? Rewrote `test/js/bun/ffi/ffi-viewSource-non-object.test.ts` to assert the error is actually thrown (the old assertions only passed because `expect().toThrow()` currently accepts a returned Error, which #34392 tightens). Added `test.each` cases for `viewSource` (non-object descriptor, unknown type, non-object options, callback descriptor, valid input) and `JSCallback` (non-object options, non-callable callback, unknown type, valid input). ``` $ bun bd test test/js/bun/ffi/ffi-viewSource-non-object.test.ts 24 pass 0 fail ``` 22 of the 24 cases fail on an unfixed build (the two valid-input cases pass as expected). The full `test/js/bun/ffi/` suite is green and clean under `BUN_JSC_validateExceptionChecks=1`. ### Related #34392 changes `toThrow()` to stop accepting a returned Error and rewrites this test to assert on the returned value as a stopgap. This PR is the actual fix; whichever lands second will need the test file reconciled. #33200 touches `FFI::print`/`FFI::callback` for a different bug (propagating exceptions from descriptor getters) and still returns the validation error as a value, so the wrappers here cover both before and after that change.  --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi-viewSource-non-object.test.ts

…ters print_callback swallowed a pending JS exception from the descriptor's args/threadsafe/returns getters and returned a non-empty "Out of memory" error value instead, tripping "host fn return/exception state mismatch". Widen it to JsResult and propagate the error so the getter's exception is thrown to JS.

FFI::callback had the same bug as print_callback: it converted Err(JsError::Thrown) from generate_symbol_for_function into a non-empty "Out of memory" error value while the real exception stayed pending, tripping "host fn return/exception state mismatch". Reachable with: new JSCallback(() => {}, { get args() { throw new Error("boom") } }) Same fix: ?-propagate so the getter's exception is thrown to JS. Also switch print()'s generate_symbols call to ?-propagation; print already returns JsResult and the unwrap_or(Some(JSValue::ZERO)) form was correct for Thrown but not for a future OutOfMemory.

robobun · 2026-07-19T23:21:04Z

Rebased onto main (99fc2f8). The test file conflicted with #34396 and other additions to ffi-viewSource-non-object.test.ts; the throwing-getter coverage is now integrated into the existing FFI viewSource / FFI JSCallback describe blocks alongside the newer tests.

Verified after rebase:

bun bd test test/js/bun/ffi/ffi-viewSource-non-object.test.ts: 33 pass
Both one-line repros from the panic report exit 0 with the user's error caught:

new JSCallback(x => x, { get args() { throw new Error("boom") } });   // caught: boom
viewSource({ get args() { throw new Error("boom2") } }, true);        // caught: boom2

Pre-rebase debug build still aborts with panic: host fn return/exception state mismatch (rc 134) on the same inputs.

claude

LGTM — focused crash fix that swaps error-swallowing .unwrap_or_else for ?-propagation at three call sites, with tests covering every entry point.

What was reviewed

print_callback's only caller is print (already JsResult<JSValue>), so widening the return type is safe; create_utf8_for_js returns JsResult<JSValue>, so the new tail expression type-matches.
Confirmed no remaining .unwrap_or_else(|_| ...) in src/runtime/ffi/; the two intentionally-excluded open/link_symbols sites use unwrap_or(Some(JSValue::ZERO)) which is correct for Err(Thrown) as the PR body explains.
Test matrix covers all four getter-read properties (args/threadsafe/returns/ptr) on both viewSource(_, true) and JSCallback, plus the non-callback generate_symbols path.
All prior inline review threads (mine and CodeRabbit's) are addressed or explicitly deferred with stated rationale in the PR body.

Extended reasoning...

Overview

This PR fixes a debug-assertion panic ("host fn return/exception state mismatch") in three bun:ffi entry points — FFI::callback (backing new JSCallback), FFI::print_callback (backing viewSource(_, true)), and the non-callback branch of FFI::print — where a pending JS exception from a descriptor property getter was being flattened into a non-empty return value via .unwrap_or_else(|_| Some(<error instance>)) or .unwrap_or(Some(JSValue::ZERO)). The fix replaces those with ?-propagation. print_callback is widened from -> JSValue to -> JsResult<JSValue>, which is safe because its sole caller (print, verified by grep) already returns JsResult<JSValue>. The final create_utf8_for_js(...).unwrap_or(JSValue::ZERO) is dropped in favor of returning the JsResult directly. Total: ~30 net Rust lines (mostly deletions) and ~50 lines of new tests integrated into the existing ffi-viewSource-non-object.test.ts.

Security risks

None. This is exception-propagation plumbing in a debug-facing FFI helper (viewSource) and a callback constructor. No new inputs are trusted, no new capabilities exposed; the change strictly removes error-swallowing so user-thrown exceptions surface instead of tripping an internal assertion.

Level of scrutiny

Low-to-moderate. The change is mechanical (.unwrap_or_else → ?) and localized to error paths that were previously broken. I verified: (1) print_callback has exactly one caller and it already handles JsResult; (2) create_utf8_for_js returns JsResult<JSValue> so the tail expression is well-typed; (3) grep confirms zero remaining .unwrap_or_else(|_| sites in src/runtime/ffi/. The intentionally-excluded open()/link_symbols() sites are correct for the reachable Err(Thrown) variant (JSValue::ZERO is the pending-exception sentinel), and the author verified this empirically on an unfixed build; the exclusion is documented in the PR body.

Other factors

This PR went through two rounds of bug-hunter review. The first round found the sibling FFI::callback site — fixed in 124eacd with matching tests. The second round asked for ptr in the test matrix (added), flagged open/link_symbols (declined with rationale now in the PR body; CodeRabbit independently withdrew the same flag), and noted a pre-existing dead threadsafe validation check (correctly deferred as an out-of-scope behavioral change). All threads are resolved. Tests are comprehensive: test.each over all four getter-read properties on both entry points, plus the non-callback generate_symbols path, asserting the exact error instance and message. The author confirmed post-rebase that the file passes (33 tests) and that the pre-fix build still panics on the repros.

Jarred-Sumner · 2026-08-01T09:09:01Z

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.)

robobun · 2026-08-01T09:36:08Z

It looks like #35246 didn't pick this up. On current main (which includes #35246), both sites this PR fixed are still present in src/runtime/ffi/ffi_body.rs:

callback() at :1300-1302:

if let Some(val) = generate_symbol_for_function(global_this, interface, func)
    .unwrap_or_else(|_| {
        Some(ZigString::init(b"Out of memory").to_error_instance(global_this))

print_callback() at :1375-1376:

if let Some(val) = generate_symbol_for_function(global, object, &mut function)
    .unwrap_or_else(|_| Some(ZigString::init(b"Out of memory").to_error_instance(global)))

Both code paths are still reached: the viewSource and JSCallback wrappers in src/js/bun/ffi.ts pass the raw options object straight to native (nativeViewSource(symbols, isCallback) and nativeCallback(options, cb)), so a throwing args/threadsafe/returns/ptr getter still leaves the exception pending while unwrap_or_else substitutes a non-empty value, tripping the assertion.

The getter-propagation tests this PR added are also not in test/js/bun/ffi/ffi-viewSource-non-object.test.ts on main.

Happy to rebase and reopen this, or fold the ?-propagation fix and tests into a fresh PR, whichever you prefer.

github-actions Bot added the claude label Jul 1, 2026

robobun mentioned this pull request Jul 1, 2026

fix: throw proper error in ByteBlobLoader when stream store is already consumed #28786

Merged

coderabbitai Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread src/runtime/ffi/ffi_body.rs

claude Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread src/runtime/ffi/ffi_body.rs

claude Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread test/js/bun/ffi/ffi-viewSource-non-object.test.ts Outdated

Comment thread src/runtime/ffi/ffi_body.rs

Comment thread src/runtime/ffi/ffi_body.rs

claude Bot reviewed Jul 1, 2026

View reviewed changes

robobun mentioned this pull request Jul 16, 2026

bun:ffi: throw validation errors from viewSource and JSCallback instead of returning them #34396

Merged

robobun added 2 commits July 19, 2026 23:13

robobun force-pushed the farm/68f904a3/ffi-viewsource-exception branch from a0fe876 to 984b431 Compare July 19, 2026 23:20

claude Bot reviewed Jul 19, 2026

View reviewed changes

Jarred-Sumner closed this Aug 1, 2026

Navigation Menu

Uh oh!

bun:ffi: propagate exceptions from viewSource callback descriptor getters - #33200

bun:ffi: propagate exceptions from viewSource callback descriptor getters#33200
robobun wants to merge 2 commits into
mainfrom
farm/68f904a3/ffi-viewsource-exception

robobun commented Jul 1, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jul 19, 2026

Uh oh!

claude Bot left a comment

Uh oh!

Jarred-Sumner commented Aug 1, 2026

Uh oh!

robobun commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

robobun commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jul 19, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Jarred-Sumner commented Aug 1, 2026

Uh oh!

robobun commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Jul 1, 2026 •

edited

Loading

robobun commented Jul 1, 2026 •

edited

Loading

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading