Fix stale exception when a lazy property initializer throws during property enumeration - #37158
Fix stale exception when a lazy property initializer throws during property enumeration#37158robobun wants to merge 1 commit into
Conversation
When a static hash-table property with a PropertyCallback throws during
reification (for example Bun.$ evaluating Symbol("cwd") after the
global Symbol was overwritten), getPropertySlot returns false with the
exception still pending. JSC__JSValue__forEachPropertyImpl skipped its
CLEAR_IF_EXCEPTION in that case, so the stale exception leaked into the
next property's host call and tripped releaseAssertNoException, aborting
the process. In release builds the stale exception made every later
static property lookup report not-found, so Bun.inspect(Bun) silently
dropped most of the Bun object.
Also remove the debug-only reportUncaughtExceptionAtEventLoop calls in
the Bun.sql lazy initializers. They ran the uncaughtException machinery
while the load error was still pending on the VM, and the
process._fatalException lookup inside it reified a property (structure
transition) and then reported it as missing because of the pending
exception, hitting the storedPrototype stale-structure assertion. The
error already propagates to the property access via RETURN_IF_EXCEPTION,
matching every other lazy property callback.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the reorder in forEachPropertyImpl matches what forEachPropertyOrdered already does at bindings.cpp:5545-5549, and the removed #if BUN_DEBUG blocks were debug-only and demonstrably wrong (they treated a propagating error as uncaught). One test-hygiene nit inline.
Extended reasoning...
Overview
Three files touched: a 4-line reorder in src/jsc/bindings/bindings.cpp (JSC__JSValue__forEachPropertyImpl), removal of two 3-line #if BUN_DEBUG blocks in src/jsc/bindings/BunObject.cpp (defaultBunSQLObject / constructBunSQLObject), and a new subprocess regression test in test/js/bun/util/inspect.test.js.
Correctness
The bindings.cpp change moves CLEAR_IF_EXCEPTION(scope) from after the continue guard to before it. This is the exact pattern the sibling forEachPropertyOrdered already uses (bindings.cpp:5545-5549: bool hasProperty = ...; scope.tryClearException(); if (!hasProperty) continue;). The old ordering meant a lazy PropertyCallback that throws (returning false from getPropertySlot) leaked its exception into the next iteration, where the next lazy callback's host wrapper release-asserts no pending exception. The reorder is a no-op on the hasProperty == true path and a strict fix on the false path.
The BunObject.cpp change deletes debug-only blocks that called reportUncaughtExceptionAtEventLoop while the module-load exception was still pending — misclassifying a propagating error (the very next line is RETURN_IF_EXCEPTION) as uncaught, and re-entering JS with a pending exception, which the PR shows trips the storedPrototype stale-structure assertion. Removing them makes these two callbacks behave like every other lazy property callback in the file (e.g. constructBunShell), and has zero effect on release builds.
Security risks
None. This is exception-handling hygiene in the inspect/enumeration path; no untrusted-input parsing, auth, or crypto is touched.
Level of scrutiny
Medium-low. bindings.cpp is a sensitive file, but the change is a mechanical reorder that mirrors an existing sibling implementation, and the BunObject.cpp change removes debug-only code. The PR description traces both failure modes precisely (debug abort + release-build silently dropping properties), and the test covers both the enumeration behavior and direct Bun.$ / Bun.sql access throwing catchably.
Other factors
One nit was flagged inline: the new test pipes stderr but never drains it, diverging from the neighboring "huge sparse array" test's Promise.all([stdout, stderr, exited]) + combined-object assertion. This is test hygiene (better failure diagnostics on regression), not a correctness issue, and doesn't block approval.
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("OK 2"); |
There was a problem hiding this comment.
🟡 The test sets stderr: "pipe" but never drains it — only proc.stdout.text() and proc.exited are awaited. Match the neighboring "huge sparse array" test and REVIEW.md's subprocess rule: const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]), then assert the combined { stdout: stdout.trim(), stderr, exitCode } object. Otherwise a regression that reintroduces the debug abort will show only received "" with the stderr backtrace silently discarded.
Extended reasoning...
What the bug is
The new test at test/js/bun/util/inspect.test.js:823-830 spawns a subprocess with stderr: "pipe" but only awaits proc.stdout.text() and proc.exited:
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
expect(stdout.trim()).toBe("OK 2");
expect(exitCode).toBe(0);The stderr pipe is opened but never read. REVIEW.md rejects this pattern explicitly under Tests reviewers reject → Subprocess tests: drain pipes concurrently: "Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) — an unread pipe fills the ~64KB OS buffer and deadlocks the child. assert a combined { stdout, stderr, exitCode } object."
Why local convention also demands it
The immediately neighboring subprocess test in the same file — "Bun.inspect huge sparse array summarizes holes without iterating them" — follows the correct pattern exactly:
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "...", stderr: "", exitCode: 0 });REVIEW.md separately requires "Match the exact file's local conventions", so this test diverges from both the repo-wide rule and the pattern established a few lines above it.
Step-by-step: what goes wrong on a regression
The whole point of this test is to catch a regression of the debug-build abort described in the PR (ASSERTION FAILED: Unexpected exception observed). Walk through what happens if that abort comes back:
- Child runs
globalThis.Symbol = 0; Bun.inspect(Bun);— the lazyBun.$callback throws, the stale exception leaks, and the debug build aborts with a JSC assertion. - The assertion message and backtrace are written to stderr.
- The child exits before printing
"OK 2"to stdout. - The parent reads
stdout→"", andexitCode→ non-zero (or a signal). - The test failure prints only:
expect(stdout.trim()).toBe("OK 2")— Expected"OK 2", Received"".
The actual diagnostic — the assertion text and backtrace on stderr — is silently discarded because nothing ever read the pipe. Whoever debugs the CI failure has no idea why the child died. With the combined-object assertion, the failure output would instead show stderr: "ASSERTION FAILED: Unexpected exception observed... <backtrace>" and exitCode: <signal>, which is exactly the signal you want.
Deadlock direction
Bun.inspect(Bun) reifies every lazy property on the Bun object; with Symbol clobbered several of these ($, sql, postgres, SQL) throw during reification. In the current passing path stderr stays empty, so the ~64KB pipe-buffer deadlock does not occur today. But the pattern is what REVIEW.md rejects regardless — if any of those reification paths (or a future warning) becomes noisy on stderr, the child could block writing to a full pipe while the parent only drains stdout.
Fix
One-line change to match the neighboring test:
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: "OK 2", stderr: "", exitCode: 0 });Severity
Nit. The test passes as written and nothing concretely breaks today — the deadlock risk is theoretical for this particular script, and the diagnostic loss only bites on a future regression. But it violates an explicit repo review rule, diverges from the immediate neighboring convention, and the fix is trivial.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Confirmed overlap: #37107 already contains the same forEachPropertyImpl change (clear the pending exception before skipping a not-found property) with a test that covers the enumeration behavior, and #37001 removes the same debug-only reportUncaughtExceptionAtEventLoop blocks in the Bun.sql initializers, plus a WebKit bump that fixes the storedPrototype stale-structure assertion at its actual source in JSObject::getPropertySlot. Between the two of them every line of this PR is covered, so closing this one in their favor. |
1 participant
Fixes a fuzzer-found abort (fingerprint
45150bc578c49ad4).Repro
The fuzzer hit this through
Bun.jest(...).expect(Bun).toBeNil(), whose failure message formats theBunobject. The globalSymbolhad been clobbered by an earlier iteration in the same fuzzer process, so reifying the lazyBun.$property threwSymbol is not a function. (In 'Symbol("cwd")', 'Symbol' is 0)from the shell builtin.Bug 1:
JSC__JSValue__forEachPropertyImplleaks the exceptionA static hash-table property with a
PropertyCallbackthat throws during reification makesgetPropertySlotreturnfalsewith the exception still pending. The enumeration loop did:so the stale exception leaked into the next property's lazy callback, whose host-call wrapper release-asserts no pending exception, aborting the process in debug/ASAN builds. In release builds the stale exception made every subsequent static-table lookup report not-found, so
Bun.inspect(Bun)silently dropped most properties (output shrinks from ~13 KB to ~1.4 KB). The fix clears before deciding to skip, matching whatforEachPropertyOrderedalready does.Bug 2:
Bun.sqlinitializer runs the uncaughtException machinery mid-reificationWith bug 1 fixed, the same repro then died in the debug-only block in
defaultBunSQLObject:This runs
Bun__handleUncaughtExceptionwhile the module-load error is still pending on the VM. Itsprocess->get(globalObject, "_fatalException")then reifies_fatalException(a structure transition) but reports it missing becausesetUpStaticFunctionSlotsees the pending exception, andJSObject::getPropertySlothits thestoredPrototypestale-structure assertion:Reproducible on its own with
globalThis.Symbol = 0; Bun.sqlin a debug build. The report call also misclassifies a propagated error as uncaught: the very next line rethrows it to the property access, where user code can catch it. Removed both blocks; the error now propagates like every other lazy property callback.Test
test/js/bun/util/inspect.test.js: tampersSymbolin a subprocess, checksBun.inspect(Bun)still contains later properties, and thatBun.$/Bun.sqlthrow catchable errors. Fails on an unfixed release build (missing properties) and an unfixed debug build (abort); passes with this change.[stamp-90s] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file