Conversation
|
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. WalkthroughThe PR removes debug exception reporting from Bun SQL lazy-property constructors and updates property enumeration to handle failed lazy initialization. New subprocess tests cover stack exhaustion and throwing property initializers. ChangesLazy property exception handling
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR fixes crashes and incorrect inspection behavior when lazy Bun properties throw, with regression coverage included; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Closing this as a duplicate of #29642. That PR already has both of these hunks (the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| bool hasProperty = object->getPropertySlot(globalObject, property, slot); | ||
| // Ignore exceptions from "Get" proxy traps and lazy property initializers. | ||
| CLEAR_IF_EXCEPTION(scope); | ||
| if (!hasProperty) | ||
| continue; |
There was a problem hiding this comment.
🔴 The slow-path prototype walk further down in this same function (line 5722: iterating = iterating->getPrototype(globalObject).getObject();) has no exception check — when iterating is a Proxy whose getPrototypeOf trap throws, getPrototype returns an empty JSValue and .getObject() dereferences a null cell, the exact JSCJSValueCell.h:92 fingerprint this PR is fixing. The fast-path twin at lines 5601–5611 already guards this with CLEAR_IF_EXCEPTION ("Ignore exceptions from Proxy getPrototype trap"); per REVIEW.md's "fix the whole class … fast/slow paths", it's worth guarding the slow-path sibling here too — split the call, CLEAR_IF_EXCEPTION(scope) after getPrototype, and let the existing while (iterating && …) condition terminate on null.
Extended reasoning...
What the bug is
JSC__JSValue__forEachPropertyImpl — the function this PR modifies at line 5646 — walks the prototype chain in two places. The fast path at lines 5601–5611 does:
if (JSValue proto = prototypeObject.getPrototype(globalObject)) {
...
}
// Ignore exceptions from Proxy "getPrototype" trap.
CLEAR_IF_EXCEPTION(scope);The slow path at line 5722 does the same operation without the guard:
iterating = iterating->getPrototype(globalObject).getObject();When iterating is a ProxyObject whose getPrototypeOf trap throws, getPrototype dispatches through the method table to ProxyObject::getPrototype, which invokes the trap, hits RETURN_IF_EXCEPTION, and returns JSValue(). Calling .getObject() on an empty JSValue: on JSVALUE64 the encoded value is 0, so isCell() is true (0 & NotCellMask == 0), asCell() returns nullptr, and JSCell::getObject() reads m_type from a null pointer. This is exactly the "member call on a null JSCell" at JSCJSValueCell.h:92 that the PR description names as the crash it is fixing.
Step-by-step reproduction
const p = new Proxy({}, { getPrototypeOf() { throw new Error(); } });
const obj = Object.create(p);
obj.x = 1;
Bun.inspect(obj); // or console.log(obj)objis a plainJSFinalObjectwith own propertyx. It passescanPerformFastPropertyEnumerationForIterationBun(noOverridesGetOwnPropertySlot, no static props, no getters), sofast = true.- The fast path processes
x, setsanyHits = true. - Line 5601:
obj.getPrototype(globalObject)is the ordinary internal-slot read (obj is not itself a Proxy) and returnsp.pis notobjectPrototype/functionPrototype, not aJSGlobalProxy(ProxyObjectis a different class), and its structure hasoverridesGetOwnPropertySlot()set, socanPerformFastPropertyEnumerationForIterationBun(p)returns false at line 633 →fast = false,prototypeObject = p,goto restart. - Slow path:
iterating = p. Thewhilecondition at line 5623 passes (pis not aJSGlobalProxy, notobjectPrototype,prototypeCount1→2 < 5). - Line 5627:
getOwnPropertyNamesonp— noownKeystrap defined, so it forwards to the target{}and returns an empty list without throwing. The property loop is a no-op. nonIndexedOnlyis false (Bun.inspectcalls the non-indexed variant), so thebreakat 5712 doesn't fire.pis not callable (its target isn't callable).p != globalObject.- Line 5722:
p->getPrototype(globalObject)invokes the throwinggetPrototypeOftrap → returnsJSValue()→.getObject()→ null-cell dereference → segfault.
There is no RETURN_IF_EXCEPTION or CLEAR_IF_EXCEPTION between line 5710 (inside the property loop, not reached with an empty property list) and line 5722.
Why existing code doesn't prevent it
The last exception check before line 5722 is the RETURN_IF_EXCEPTION at line 5710, which is inside the for (auto& property : properties) loop and never runs when the property list is empty. The while condition at line 5623 does check iterating &&, but that only helps on the next iteration — the crash happens inside .getObject() before control returns to the loop head. The trailing if (scope.exception()) at line 5728 is after the loop and never reached.
Impact
User-reachable segfault via Bun.inspect / console.log on any object whose prototype chain contains a Proxy with a throwing getPrototypeOf trap. This is the same crash signature (JSCJSValueCell.h:92) the PR is fixing 76 lines above.
Why this belongs in this PR
REVIEW.md, "Correctness: the bug class, not the bug": "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern: … fast/slow paths." Line 5722 is the literal slow-path twin of the fast-path call at 5601–5611, which already has the guard and the comment "Ignore exceptions from Proxy getPrototype trap." It is in the same function, the same while loop's neighborhood, and produces the identical crash fingerprint the PR description names. This is pre-existing (the PR did not introduce it), but it falls squarely under the same-class rule.
Fix
Split the call and clear the exception, matching the fast path:
JSValue proto = iterating->getPrototype(globalObject);
// Ignore exceptions from Proxy "getPrototype" trap.
CLEAR_IF_EXCEPTION(scope);
iterating = proto ? proto.getObject() : nullptr;The existing while (iterating && …) condition then terminates the loop cleanly on the next check.
| expect(rest).toEqual({ archive: true, $: "function" }); | ||
| // On Windows the formatter's own stack headroom is larger than JSC's, so the | ||
| // initializer never gets to fail there. | ||
| if (!isWindows) expect(shell).toBe(false); |
There was a problem hiding this comment.
🟡 The if (!isWindows) expect(shell).toBe(false) assertion depends on the ratio between the native stack consumed by getPropertySlot → setUpStaticPropertySlot → constructBunShell → JSC::call and one JS recurse() frame — the !isWindows gate already concedes this ratio flips on one platform, and it can plausibly flip on release/aarch64/JIT lanes too (the PR body notes only debug was verified locally). Since the load-bearing regression check is archive: true + stderr === "" + exitCode === 0, consider dropping the shell assertion or gating it on isDebug so a lane where the initializer happens to succeed at the same frame does not turn the test red on correct code.
Extended reasoning...
What the assertion encodes
The spawned script recurses until the stack overflows, then on each unwind frame retries Bun.inspect(Bun, {depth: 0}) until it first succeeds. At that first-success frame, the available stack headroom H lies in the interval [T_inspect, T_inspect + one_JS_frame) — where T_inspect is the minimum headroom for forEachPropertyImpl to pass vm.isSafeToRecurse() (bindings.cpp:5490) and finish enumerating, and one_JS_frame is the size of one recurse() activation.
For shell === false to hold, constructBunShell's JSC::call(globalObject, createShellFn, ...) re-entry must still overflow at that same frame, i.e. H < T_shell where T_shell is the headroom needed for the interpreter re-entry to pass its own soft-stack check. That requires T_shell − T_inspect ≥ one_JS_frame. In words: the native stack between the top of forEachPropertyImpl and the interpreter re-entry inside constructBunShell must be larger than one JS frame.
Why that ratio is not stable
T_shell − T_inspect is the stack consumed by getPropertySlot → setUpStaticPropertySlot → constructBunShell (three JSFunction::create + a MarkedArgumentBuffer) → JSC::call → Interpreter::executeCall → vmEntryToJavaScript. That native span is set by the compiler and the target: release inlines and shrinks the chain relative to debug, aarch64 and x64 lay frames out differently, and ASAN inflates every frame. one_JS_frame is set by the execution tier — an LLInt recurse() frame is smaller than a baseline-JIT one, so once recurse tiers up, unwinding one frame reclaims more headroom and the window may be jumped in a single step.
The !isWindows gate on line 109 is direct evidence this is not hypothetical: the comment says "On Windows the formatter's own stack headroom is larger than JSC's, so the initializer never gets to fail there" — i.e. on one supported platform T_shell ≤ T_inspect + one_JS_frame and shell comes back true. The PR body's evidence footer says "Platform-specific test(s) that do not run on this machine. Deferring to CI", so the remaining non-Windows lanes (release × x64/aarch64 × ASAN) were not verified locally.
Step-by-step failure
- On some non-Windows lane (say release aarch64), the inlined
constructBunShellchain consumes ~1.5 KB of native stack, and a tiered-uprecurse()frame is ~2 KB. - The script unwinds to the first frame where
Bun.inspect(Bun)succeeds; headroomH ≈ T_inspect + 1.8 KB. forEachPropertyImplenumerates$;constructBunShellneeds only ~1.5 KB more to reachvmEntryToJavaScript, which passes its soft-stack check →$initializes successfully →out.includes("$:")istrue.archiveis alsotrue(nothing threw, so nothing to leak),stderris empty,exitCodeis 0 — the fix is exercised correctly and the runtime is behaving.expect(shell).toBe(false)fails; the test goes red on correct code.
What is actually load-bearing
The regression this PR fixes is: when $'s initializer throws inside getPropertySlot, the pending exception used to leak into the next iteration and either abort (releaseAssertNoException in the next Rust prop callback) or make setUpStaticFunctionSlot skip every following static property. The assertion that distinguishes fixed from unfixed on all builds is archive: true (proves properties after $ are still enumerated) plus stderr === "" / exitCode === 0 (proves no abort). shell: false is the setup-precondition check — it proves the initializer did throw so archive: true isn't vacuous — but as written it encodes a frame-size assumption that already needed one platform carve-out.
Suggested fix
Either drop the shell assertion entirely (the PR description already states archive: true distinguishes fixed from unfixed on release builds), or gate it more tightly — e.g. if (isDebug && !isWindows) — where the un-inlined native chain reliably exceeds one JS frame. Per REVIEW.md's "branch per-platform rather than dropping precision" and the RSS-threshold guidance to branch on isASAN/isDebug, narrowing the gate is the smallest change that keeps the precondition check without risking a red lane on correct code.
1 participant
What does this PR do?
Fuzzing found a crash while formatting the
Bunobject close to the stack limit (expect(Bun).toBeFalse()inside a recursion that had just overflowed). The fingerprint isJSValue::getObject()being called on an empty value (JSCJSValueCell.h:92, member call on a nullJSCell). Two things go wrong when one ofBun's lazy static properties throws while it is being initialized, which a stack overflow makes easy to trigger for the ones that run JavaScript (Bun.$,Bun.sql,Bun.postgres,Bun.SQL):defaultBunSQLObject/constructBunSQLObject(BunObject.cpp) had a debug-onlyreportUncaughtExceptionAtEventLoopcall in front of theirRETURN_IF_EXCEPTION. The exception is not uncaught at that point (it is about to be propagated to whoever touchedBun.sql), and the uncaught-exception machinery runsprocess._fatalExceptionlookups and the default error printer with the exception still pending. Depending on the state of the process object that either trips JSC assertions, or clears the exception, in which caseRETURN_IF_EXCEPTIONno longer fires andsqlValue.getObject()runs on the empty valuerequireIdreturned. That is the fuzzer's crash. Either way the script's owncatchwas also undermined: the error got printed and the exit code set to 1. The debug block is removed; the error simply propagates like it does in release builds.JSC__JSValue__forEachPropertyImpl(bindings.cpp, used byBun.inspect,console.logand theexpectfailure messages) didcontinuewhengetPropertySlotreturned false, before itsCLEAR_IF_EXCEPTION. When the slot was missing because the initializer threw, the exception stayed pending while the loop went on to the next property. In debug and ASAN builds the next Rust-backed lazy property then aborts into_js_host_call(Unexpected exception observed), which is what the reproducer hits on a current debug build. In release buildssetUpStaticFunctionSlotsees the stale exception and reports every following not-yet-initialized static property as missing, soBun.inspect(Bun)silently drops everything between$and the first property that had already been initialized. The exception is now cleared before thecontinue, matching what the fast path andforEachPropertyOrderedin the same file already do.How did you verify your code works?
releaseAssertNoExceptioninBunObject_lazyPropCb_Archive); with this change it runs to completion and exits with the expectedtoBeFalseerror, with nothing from the sanitizers or assertions.test/js/bun/util/BunObject.test.ts. Each recurses until the stack overflows and then touchesBun.sqlor callsBun.inspect(Bun)while unwinding. Without this change the first one aborts on debug builds (it passes on release builds, since the code it covers is debug-only), and the second one aborts on debug builds and fails on release builds becauseArchive:is missing from the output. Both pass with the change, in well under a second each on a debug build.test/js/bun/util/inspect*.test.js,test/js/bun/console/,test/js/node/util/and the nodetest-util-inspect-proxy.js/getters-accessing-this.jstests still pass.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/BunObject.test.ts