Conversation
In the slow path of JSC__JSValue__forEachPropertyImpl, a getPropertySlot that returned false because the lookup threw (a Proxy get trap, or a static-table lazy property whose initializer entered JS and hit a stack overflow) skipped the CLEAR_IF_EXCEPTION, so the exception stayed pending for the rest of the walk. Every later static-table entry then reported itself as missing, the next lazy property callback ran with an exception already set (assertion in debug builds), and a Proxy further up the chain returned an empty prototype that was dereferenced as an object. Clear the exception whether or not the property was found, and stop walking when getPrototype returns empty instead of calling getObject() on it, which also covers a getPrototypeOf trap that throws on its own. Bun.sql / Bun.SQL getters no longer report a failed module load through the uncaught exception handler in debug builds while that exception is still pending; it is propagated to the caller like in release builds.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesException safety
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change clears stale exceptions during property formatting and prevents invalid prototype handling, with targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Nothing to act on from the automated review (no line comments). CI for this branch is still running; I will follow up here if anything fails. |
|
Updated 7:02 PM PT - Aug 16th, 2026
❌ @robobun, your commit 6a2e675 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 39380That installs a local version of the PR into your bun-39380 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSValue prototype = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" traps. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (!prototype) | ||
| break; | ||
| iterating = prototype.getObject(); |
There was a problem hiding this comment.
🟡 Same-class sibling: src/jsc/bindings/napi.cpp:2080 still does owner->getPrototype(globalObject).getObject() with no exception check between getOwnPropertyDescriptor and getPrototype — a Proxy on the prototype chain with a throwing trap produces the same empty-JSValue → null-cell segfault this hunk fixes. Requires a native addon calling napi_get_all_property_names with napi_key_include_prototypes + a filter flag, so a follow-up is fine, but per REVIEW.md ("grep for every sibling site sharing the pattern") it's worth either applying the same guard here or noting the intentional exclusion.
Extended reasoning...
What the bug is
This PR fixes iterating->getPrototype(globalObject).getObject() in bindings.cpp because a throwing Proxy getPrototypeOf trap makes getPrototype return an empty JSValue, and .getObject() on an empty JSValue reads JSCell::m_type off a null cell pointer — the "segfault at address 0x5" described in the PR body.
The identical pattern remains at src/jsc/bindings/napi.cpp:2080, inside napi_get_all_property_names when key_mode == napi_key_include_prototypes and key_filter selects by descriptor attributes (enumerable/writable/configurable):
while (!owner->getOwnPropertyDescriptor(globalObject, propKey, desc)) {
JSObject* proto = owner->getPrototype(globalObject).getObject();
if (!proto) break;
owner = proto;
}There is no exception check between getOwnPropertyDescriptor (which invokes a Proxy getOwnPropertyDescriptor trap and can throw, returning false) and getPrototype (which invokes a Proxy getPrototypeOf trap and can throw, returning empty).
Step-by-step proof
- A native addon calls
napi_get_all_property_names(env, obj, napi_key_include_prototypes, napi_key_enumerable, napi_key_numbers_to_strings, &result)on:Object.create(new Proxy({}, { ownKeys: () => ['foo'], getOwnPropertyDescriptor: () => { throw new Error('boom'); }, }))
collectInheritedPropertyKeyssucceeds — withDontEnumPropertiesMode::Includeit only invokes theownKeystrap (which returns['foo']), not the descriptor trap.- The filter loop reaches
'foo'.obj(a plainObject.createresult) has no own descriptor for it, so thewhileclimbs to the Proxy prototype and callsowner->getOwnPropertyDescriptor(...). ProxyObject::getOwnPropertyDescriptorinvokes the trap, which throws. The function returnsfalsewith an exception pending.- The loop body immediately calls
owner->getPrototype(globalObject).ProxyObject::getPrototypehits its earlyRETURN_IF_EXCEPTIONand returns an empty JSValue (encoded 0). .getObject()on encoded 0:isCell()is true (0 & NotCellMask == 0), so it callsasCell()->getObject(), which readsm_typeat offset 5 off a nullthis— segfault at address 0x5, exactly the crash this PR fixes in the formatter.
The same crash is also reachable directly via a throwing getPrototypeOf trap on the Proxy, without needing getOwnPropertyDescriptor to throw first.
Why existing code doesn't prevent it
The if (!proto) break; on the next line checks whether getObject() returned nullptr (i.e., the prototype was a non-object like null), but that check is on the result of .getObject() — the null-cell dereference happens inside .getObject() before that guard is ever reached. There is no RETURN_IF_EXCEPTION or CLEAR_IF_EXCEPTION anywhere in this loop.
Impact
A native addon that enumerates inherited properties with an attribute filter on an object whose prototype chain contains an adversarial Proxy segfaults the process. The surface is much narrower than the formatter path (requires a native addon + napi_key_include_prototypes + a filter flag + hostile JS), so it does not carry the same urgency, but it is the same release-build crash.
How to fix
Apply the same guard this PR adds in bindings.cpp: split the chained call, check for exception (either NAPI_RETURN_IF_EXCEPTION or clear + break, matching whatever the surrounding N-API function already does for exceptions), and bail on an empty JSValue before calling .getObject().
Why this is flagged
REVIEW.md, Correctness: the bug class, not the bug: "Fix the whole class in the same PR — grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR." A grep for getPrototype(globalObject).getObject() finds this site directly. This is pre-existing and in a different subsystem, so keeping the PR scoped to the formatter and doing this as a follow-up is entirely reasonable — just noting it so it isn't lost.
|
On the review note about napi.cpp: agreed, the attribute filter loop in napi_get_all_property_names has the same getPrototype().getObject() pattern, and nothing in that loop checks for exceptions (get, toPropertyKey, getOwnPropertyDescriptor, getPrototype, push). It is only reachable from a native addon using napi_key_include_prototypes together with an attribute filter, and it needs an addon based test, so it is being handled as a separate change rather than here. This PR stays scoped to the formatter walk, which was the only other site the grep turned up. |
|
This has the same two forEachPropertyImpl hunks and the same BunObject.cpp deletion as #30245, which has been open since May, is merged up to current main, and has been through review. Unless there is something here that #30245 lacks, closing this one in favour of #30245 (or of #37175, which is a superset of both and also fixes the util.inspect lazy initializer) would leave the maintainers one thing to pick. |
|
Closing as a duplicate of #29642, which fixes the same stale-exception / getPrototype null dereference in forEachPropertyImpl (the same two hunks in bindings.cpp) and now also carries the BunObject.cpp debug-block removal and the consolidated regression tests. |
…p or getPrototypeOf throws during the property walk (#29642) ### Problem - `Bun.inspect()`, `console.log()` and `expect()` failure output crash with `Segmentation fault at address 0x5` (debug builds: UBSan "member call on null pointer of type 'JSC::JSCell'" in `JSCJSValueCell.h`) when a property lookup throws while an object is being formatted. Release repro: ```js const proto = new Proxy({ a: 1 }, { getPrototypeOf() { throw new Error("boom"); } }); console.log(Object.create(proto)); ``` and likewise with a Proxy whose `get` trap (or a getter reached through a Proxy) throws for one property. - The same happens with a lazily initialized property of the `Bun` object whose initializer throws (fuzzer sample: `globalThis.Symbol` replaced, then `Bun.inspect(Bun)`; the builtin behind `Bun.$` calls `Symbol("cwd")`). Release builds print `Bun` with most of its properties missing (72 of 115 on the shipped build); debug builds abort with `ASSERTION FAILED: Unexpected exception observed` / `Symbol is not a function. (In 'Symbol("cwd")' ...)` when the next lazy property is built. - Plain-JavaScript variant of the same leak: `console.log` of a module namespace during an import cycle, when one export is still in its temporal dead zone, throws `ReferenceError: Cannot access 'x' before initialization` out of `console.log` (the stale exception is picked up while the next export is formatted). `util.inspect` prints such an export as `<uninitialized>`. - Cause, in the slow path of `JSC__JSValue__forEachPropertyImpl` (`src/jsc/bindings/bindings.cpp`): - `object->getPropertySlot()` reports a throwing Proxy trap, a throwing lazy initializer or a TDZ namespace export as "not found" with the exception still pending, and the loop `continue`d before the `CLEAR_IF_EXCEPTION` below it. The following lookups and formatting callbacks then run with that exception pending (the dropped properties, the rethrown `ReferenceError`, the debug assertion). - When the walk moves to the next prototype, `iterating->getPrototype(globalObject).getObject()` runs `getObject()` on the empty `JSValue` that `getPrototype` returns when it threw (either because of the stale exception above or because the `getPrototypeOf` trap itself throws). The empty value passes `isCell()`, so this reads the type byte of a null cell: the fault at address 5. - `napi_get_all_property_names` (`src/jsc/bindings/napi.cpp`, descriptor filter loop) has the same `getPrototype().getObject()` chain after an unchecked `getOwnPropertyDescriptor`, so a Proxy trap throwing there returned `napi_ok` with an exception pending in own-only mode and segfaulted in include-prototypes mode. - `defaultBunSQLObject` / `constructBunSQLObject` (`src/jsc/bindings/BunObject.cpp`) had a debug-only block that handed a sql module load failure to `reportUncaughtExceptionAtEventLoop` while the exception was still pending on the VM, so `globalThis.Symbol = NaN; Bun.sql` (and the fuzzer sample above, once the walk gets past `Bun.$`) aborted debug builds with `ASSERTION FAILED: ... object->structure() == this` in `Structure::storedPrototype` instead of throwing. ### Fix - `bindings.cpp`: clear the exception after `getPropertySlot` regardless of its result, which is what the ordered variant `JSC__JSValue__forEachPropertyOrdered` already does; read the next prototype into a `JSValue`, clear the exception and stop the walk when it is empty. (An earlier revision also held the prototype being walked under an `EnsureStillAliveScope`; dropped, since the raw pointer is used after every call into JS in the loop body and so is live across them anyway.) - Behaviour change to note: a property whose lookup throws is now left out of the output instead of the whole `console.log` / `Bun.inspect` call throwing or crashing. For TDZ namespace exports this differs from `util.inspect`'s `<uninitialized>`; printing that marker would be a formatter feature on top of this fix and is not attempted here. - `napi.cpp`: check for an exception after `getOwnPropertyDescriptor` and after `getPrototype` and return `napi_pending_exception`, which is what Node returns for these cases. - `BunObject.cpp`: drop the debug-only report. The exception is propagated to the reader by the `RETURN_IF_EXCEPTION` right below it, so debug builds now behave like release builds (`Bun.sql` throws). - Why this is the right place: the formatter deliberately swallows errors thrown by individual properties (getters, traps) and prints the rest of the object; these two sites were the only ones in the walk that acted on a "not found" result or a prototype value before clearing the exception that produced it. Skipping just the property (or stopping at just the prototype) whose lookup threw is the existing behaviour for every other throw site in this function. - Verification: - `test/js/bun/util/inspect.test.js`, "Bun.inspect when a property lookup throws" (5 spawned cases): a Proxy `get` trap, a getter behind a Proxy, a `getPrototypeOf` trap, a throwing lazy `Bun` property, and a two-file import cycle with a TDZ export. Without the fix (shipped release build and an unfixed debug build) all five fail: the Proxy children segfault / fail UBSan, the `Bun` child prints `[false,false,...]` on release and aborts on debug, the cycle child exits 1 with the `ReferenceError`; with it each prints everything except the one property whose lookup threw. - `test/js/bun/util/BunObject.test.ts`, "a lazy property whose builtin fails to load throws from the read": `Bun.$` / `sql` / `SQL` / `postgres` with `Symbol` broken throw a `TypeError` on two consecutive reads. Aborts on an unfixed debug build (the `BunObject.cpp` hunk); passes on release either way, as the removed block is debug-only. The fixture builds `process.env` before breaking `Symbol` because the `$` builder reads it, and building it on Windows reifies another `Bun` property mid-lookup, which on a Windows debug build would hit the separate `storedPrototype` assertion that #37001 fixes (verified on Linux only). - `test/napi/napi.test.ts`: `getOwnPropertyDescriptor` trap throwing in own-only and include-prototypes mode (compared against Node), and a `getPrototypeOf` trap that throws on the second call so the check after `getPrototype` is the one that fires. - Repros above and the tests also run clean under `BUN_JSC_validateExceptionChecks=1`. ### Background - `forEachPropertyImpl` is the property walk behind Bun's native formatter. It collects the property names of the object and of up to five prototypes, looks each one up through the original object with `getPropertySlot`, and hands the value to a callback that formats it. Errors thrown by individual properties are swallowed on purpose so that one bad getter does not make `console.log` throw. - A JSC exception is "pending" state on the VM, not C++ unwinding. A function that throws returns a failure value (`false`, or the empty `JSValue`) and leaves the exception on the VM; until something clears or rethrows it, most JSC entry points return early as soon as they are called, and debug builds assert when a function that did not throw is observed returning with an exception pending. `CLEAR_IF_EXCEPTION` drops the pending exception. - The empty `JSValue` (`JSValue()`) is encoded as 0. `isCell()` is true for it, so `getObject()` on it dereferences a null cell pointer rather than returning null; callers have to test the value itself first. - Lazy properties of the `Bun` object are entries in a static property table whose value is produced by a builder the first time the property is read (`PropertyCallback`). Some builders evaluate built-in JavaScript modules (the shell for `Bun.$`, the sql module for `Bun.sql`), so they can throw when that module fails to evaluate, and JSC reports that to the reader as "property not found" plus a pending exception. ### Consolidated duplicates Found repeatedly by the fuzzer (fingerprint `d678cafe50a2ad6e`). Earlier round, folded in here in April: #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325. This round, closed in favour of this PR: #30099 #30245 #37160 #37175 #37213 #37256 #37428 #38700 #38921 #39363 #39365 #39380 #39412 #39413 (and #39411, closed earlier). The `BunObject.cpp` hunk and the `BunObject.test.ts` test come from #30245 / #37428; the same hunk is also part of #37001, which fixes the underlying `storedPrototype` assertion in JSC. Related fixes that are not part of this bug and stay open on their own: #39382 (a custom inspect function when `node:util` fails to load), #37202 (`util.isError` with a throwing `getPrototypeOf` trap), #37331 (`forEachPropertyOrdered` when the callback throws), #37001 (stale structure in `JSObject::getPropertySlot`), #32263 (additional checks in the same napi loop). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · 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 test/napi/napi.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
1 participant
What does this PR do?
Found by the fuzzer. Formatting an object (
Bun.inspect,console.log, the "received" value in a failingexpect()message) walks its properties inJSC__JSValue__forEachPropertyImplinbindings.cpp. In the slow path, whengetPropertySlotreturned false the loop didcontinuebefore reachingCLEAR_IF_EXCEPTION. If the lookup returned false because it threw, the exception stayed pending for the rest of the walk. Two ways to get there:Bun.$is the first entry in the Bun object's table and builds itself by calling a builtin, so formattingBunright after a stack overflow makes that lookup throwMaximum call stack size exceeded(the fuzzer got there viaexpect(Bun).toHaveBeenCalledTimes()inside the catch of a runaway recursion).gettrap throws for one key.With the exception left pending, JSC's
setUpStaticFunctionSlotreports every later static-table entry as missing (so in release builds most of the Bun object silently disappeared from the output), the next lazy property callback runs with an exception already set (thereleaseAssertNoExceptionfailure in the fuzzer report), and a Proxy further up the chain returns an empty value fromgetPrototype, which the loop then called.getObject()on (segfault at address 0x5 in release builds).Changes, all in the slow path of
forEachPropertyImpl:CLEAR_IF_EXCEPTION(scope)now runs aftergetPropertySlotwhether or not the property was found, same asforEachPropertyOrderedalready did.getPrototypebefore callinggetObject()on it and stops walking if it is empty. This is the line that segfaulted in release builds with a stale exception, and it also segfaulted on its own for a prototype Proxy whosegetPrototypeOftrap throws.BunObject.cpp: once the walk gets pastBun.$, the same script reached theBun.sql/Bun.SQLgetters, which in debug builds passed a failed module load toreportUncaughtExceptionAtEventLoopwhile that exception was still pending, and the uncaught exception handler then tripped aStructure::storedPrototypeassertion looking upprocess._fatalException. The debug-only block is removed; the exception is propagated to the caller through theRETURN_IF_EXCEPTIONright below it, which is what release builds already did.How did you verify your code works?
Tests added to
test/js/bun/util/inspect.test.js("property lookups that throw while formatting"), each running in a subprocess because the old behavior takes the process down:gettrap: segfault before, prints the other keys aftergetPrototypeOftrap: segfault before, prints the own-chain keys afterBun.inspect(Bun)in the catch of a stack overflow: before, the output lostArchive,versionand everything else that follows a throwing entry (and debug builds hit the assertion); after, those are present andBun.$still initializes afterwardsAll three fail with
USE_SYSTEM_BUN=1 bun testand pass withbun bd test. The fuzzer script (with the expect call limited to the deepest few hundred frames, since every frame formats the whole Bun object) aborts on the prebuilt debug binary and exits 0 on the fixed one, also withBUN_JSC_validateExceptionChecks=1. The rest ofinspect.test.js,bun-inspect.test.ts,custom-inspect.test.js, the nodeutil-inspect-proxytests andBunObject.test.tspass with the debug build.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/util/inspect.test.js