Fix crash and dropped properties in Bun.inspect when a property lookup or getPrototypeOf throws during the property walk

Merged
RO
3mo ago
+248-13

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

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