Skip to content

Fix crash when a property lookup throws while an object is being formatted - #37428

Open
robobun wants to merge 2 commits into
mainfrom
farm/6db764f3/foreachproperty-stale-exception
Open

Fix crash when a property lookup throws while an object is being formatted#37428
robobun wants to merge 2 commits into
mainfrom
farm/6db764f3/foreachproperty-stale-exception

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What

Fixes a crash in the object property walk used by Bun.inspect, console.log and the expect() failure messages (JSC__JSValue__forEachPropertyImpl in src/jsc/bindings/bindings.cpp), found by the fuzzer. The crash needs a property lookup that throws while the object is being formatted.

Release builds segfault (Segmentation fault at address 0x5) on either of these:

const proto = new Proxy({ a: 1, b: 2, c: 3 }, {
  get(t, k, r) { if (k === "a") throw new Error("boom"); return Reflect.get(t, k, r); },
});
Bun.inspect(Object.create(proto));

const proto2 = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("boom"); } });
Bun.inspect(Object.create(proto2));

Debug builds additionally abort with releaseAssertNoException on the fuzzer's case, which is a lazy property on the Bun object throwing while Bun is formatted (the shell builtin behind Bun.$ calls the global Symbol, which a previous REPRL script had replaced). In release builds that case silently drops every property of Bun that comes after the throwing one.

Root cause

In the slow path of the walk, a getPropertySlot that returned false was skipped with continue before the CLEAR_IF_EXCEPTION that follows it. JSC reports a throwing Proxy trap or a throwing static lazy property initializer exactly that way (returns false, exception pending), so the exception stayed on the VM:

  • every later lookup on the object fails early because of the stale exception (the silently dropped properties in release, the assertion in debug when the next lazy initializer returns a value with an exception still pending),
  • when the walk moves to the next prototype, getPrototype() bails out and returns the empty JSValue, and .getObject() on it dereferences null (an empty JSValue passes isCell(), and JSCell::getObject reads the type byte at offset 5). A Proxy whose getPrototypeOf trap throws reaches this line directly, without a stale exception.

Fix

bindings.cpp: clear the exception after getPropertySlot regardless of its result (this is what the ordered variant JSC__JSValue__forEachPropertyOrdered already does), and check the value returned by getPrototype before calling getObject() on it, ending the walk if the trap threw.

Not in this PR: napi_get_all_property_names (src/jsc/bindings/napi.cpp, the descriptor filter loop) has the same getPrototype().getObject() chain and also ignores a throwing getOwnPropertyDescriptor right before it. It is only reachable from a native addon and needs its own addon-based test, so it is being fixed separately.

BunObject.cpp: defaultBunSQLObject / constructBunSQLObject had a debug-only block that passed a load failure of the SQL module to reportUncaughtExceptionAtEventLoop while the exception was still pending on the VM. Every other caller clears the exception first. The report runs process.get("_fatalException"), which reifies a static property on process with the exception pending and trips the Structure::storedPrototype assertion, so globalThis.Symbol = NaN; Bun.sql aborted debug builds, and so did the fuzzer's case once the walk got past Bun.$. The exception is propagated to the caller right below this block anyway (Bun.sql throws it), so the block is removed rather than reordered.

Tests

Two tests added to test/js/bun/util/inspect.test.js under "exceptions thrown while walking properties", both spawning a subprocess because the unfixed binary dies:

  • the two Proxy cases above. On the unfixed build the subprocess segfaults; fixed, the throwing property is skipped and the others are printed.
  • the fuzzer's case, reduced to one property: a Proxy around Symbol makes only the Symbol("cwd") call in the shell builtin throw, so Bun.$ fails to reify while Bun.inspect(Bun) runs. Unfixed release prints false false true (Archive, the property after $, is missing), unfixed debug aborts; fixed, it prints false true true. Verified on Linux and Windows (an earlier version of this test replaced Symbol entirely, which also broke loading node:util; Windows needs that to format process.env, and a lazy initializer failing there hits an unrelated pre-existing abort, so the test failed on the Windows lanes of the first CI run).

Also ran the full inspect.test.js, BunObject.test.ts, mock-fn.test.js and sql/adapter-override.test.ts against the debug build, and the repros with BUN_JSC_validateExceptionChecks=1.


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/util/inspect.test.js

JSC__JSValue__forEachPropertyImpl left the exception pending when
getPropertySlot returned false because a lazy property initializer or a
Proxy trap threw. Every later lookup on the object then failed because of
the stale exception, and walking to the next prototype called getObject()
on the empty value returned by a throwing getPrototype, a null
dereference. The same null dereference was reachable directly through a
Proxy whose getPrototypeOf trap throws.

Also drop the debug-only reporting of Bun.sql module load failures, which
reported the exception while it was still pending on the VM and tripped a
Structure assertion in debug builds.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Inspection exception safety

Layer / File(s) Summary
Exception-safe property and prototype traversal
src/jsc/bindings/bindings.cpp
Property and prototype lookup exceptions are cleared. Failing properties are skipped, and invalid prototypes stop traversal.
Inspection regression coverage
test/js/bun/util/inspect.test.js
Subprocess tests cover throwing proxy traps and lazy initializers. They verify preserved properties, clean stderr, and successful exits.
Bun SQL loader exception reporting cleanup
src/jsc/bindings/BunObject.cpp
Debug-only uncaught-exception reporting was removed from both Bun SQL loaders. Existing exception propagation remains.

Possibly related PRs

  • oven-sh/bun#37107: Modifies the same property enumeration code and adds related Bun.inspect regression tests.
  • oven-sh/bun#37234: Extends related exception handling in BunObject.cpp, bindings.cpp, and inspect.test.js.
  • oven-sh/bun#37309: Covers related exception handling to prevent inspection failures from leaking exceptions or truncating output.

Suggested reviewers: dylan-conway, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly describes the primary fix: preventing crashes during property lookup while formatting objects.
Description check ✅ Passed The description explains the cause, fix, scope, affected APIs, tests, and verification, although its headings differ slightly from the template.

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

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

The automated review above was rate limited and did not look at the diff, so nothing to address from it. CI for this branch is still running; I will follow up if anything in it fails.

Comment thread src/jsc/bindings/bindings.cpp
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

On the napi_get_all_property_names site (napi.cpp:2072): agreed that it is the same null dereference, plus the unchecked getOwnPropertyDescriptor throw right before it. It is being fixed separately, since it needs its own addon-based test under test/napi and this PR does not otherwise touch N-API. I have noted it in the description so the exclusion is on record.

Replacing the whole Symbol global also broke node:util, which Windows
needs to format process.env, so the subprocess died for an unrelated
reason there. Intercept just the Symbol("cwd") call the shell builtin
makes instead.
@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:38 PM PT - Aug 10th, 2026

@robobun, your commit f548930f4e570decb90c02e5b5caab42e7d1ee3c passed in Build #91999! 🎉


🧪   To try this PR locally:

bunx bun-pr 37428

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

bun-37428 --bun

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Nothing to act on from the automated summaries. State of the PR: the only review finding (the napi site) is tracked separately as noted above. The second commit only changes the test fixture, which previously replaced the Symbol global outright and failed on the Windows lanes for an unrelated reason (details in the description); the fix itself is unchanged. CI is running again on that commit.

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