Fix debug assertion crashes when lazy Bun properties throw during property enumeration - #37307
Fix debug assertion crashes when lazy Bun properties throw during property enumeration#37307robobun wants to merge 3 commits into
Conversation
…perty enumeration When a static lazy property callback throws (Bun.$ and Bun.sql evaluate internal modules at first access), getPropertySlot reports the slot as not found while the exception stays pending. forEachPropertyImpl continued to the next property without clearing it, so the stale exception tripped exception scope assertions in debug builds. Also drop the debug-only uncaught exception reporting from the Bun.sql lazy property callbacks. It ran the uncaught exception machinery re-entrantly in the middle of a static property lookup, mutating the Bun object mid-walk and tripping a stale structure assertion in JSObject::getPropertySlot, and the exception it reported propagates to the caller anyway.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Ignore exceptions from "Get" proxy traps and static lazy | ||
| // property callbacks, which report the slot as not found. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Condensed the flagged comment to one line in 65020da, matching the other "Ignore exceptions from ..." notes in this function. To be clear, the note is not justifying a workaround: moving CLEAR_IF_EXCEPTION above the not-found check is the fix itself. The swallowing of getter and trap exceptions is pre-existing, intentional behavior of the inspect walk, and the comment records the Lookup.cpp contract (a throwing lazy property callback reports the slot as not found) that made the original bug easy to miss. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
WalkthroughThe PR removes debug-only exception reporting from Bun SQL lazy-property constructors. It updates property enumeration to handle lookup exceptions and adds a regression test for inspecting ChangesException handling and inspection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Line 591: Update the test command around Bun.sql so it records whether
accessing Bun.sql throws after globalThis.Symbol is set to NaN, and exits or
asserts failure before printing "ok" when no exception occurs. Preserve the
success output only when the intended lazy-property failure is observed, and
avoid silently swallowing the catch result.
- Around line 587-597: Update the subprocess test around Bun.spawn and its
Promise.all to concurrently consume proc.stderr.text() alongside
proc.stdout.text() and proc.exited, preserving the existing stdout and exit-code
handling while ensuring the configured stderr pipe is drained.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6982dedd-9db9-46d9-a25d-615bfc302521
📒 Files selected for processing (3)
src/jsc/bindings/BunObject.cppsrc/jsc/bindings/bindings.cpptest/js/bun/util/inspect.test.js
💤 Files with no reviewable changes (1)
- src/jsc/bindings/BunObject.cpp
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Drain proc.stderr before waiting for process exit.
Line 595 configures stderr as a pipe, but Line 597 reads only stdout and proc.exited. If the child reproduces the debug or ASAN failure and writes enough diagnostics, the stderr pipe can fill and block the child. The test can then hang instead of reporting the failure.
Read proc.stderr.text() in the same Promise.all as stdout and proc.exited.
As per coding guidelines, subprocess tests must drain stdout, stderr, and process exit concurrently.
Proposed fix
- const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+ const [stdout, , exitCode] = await Promise.all([
+ proc.stdout.text(),
+ proc.stderr.text(),
+ proc.exited,
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await using proc = Bun.spawn({ | |
| cmd: [ | |
| bunExe(), | |
| "-e", | |
| `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`, | |
| ], | |
| env: bunEnv, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| await using proc = Bun.spawn({ | |
| cmd: [ | |
| bunExe(), | |
| "-e", | |
| `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`, | |
| ], | |
| env: bunEnv, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, , exitCode] = await Promise.all([ | |
| proc.stdout.text(), | |
| proc.stderr.text(), | |
| proc.exited, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/util/inspect.test.js` around lines 587 - 597, Update the
subprocess test around Bun.spawn and its Promise.all to concurrently consume
proc.stderr.text() alongside proc.stdout.text() and proc.exited, preserving the
existing stdout and exit-code handling while ensuring the configured stderr pipe
is drained.
Source: Coding guidelines
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that Bun.sql triggers the intended lazy-property failure.
Line 591 discards the catch result. If Bun.sql does not throw after Symbol is clobbered, the child still prints "ok" and exits successfully. The test can then pass without exercising the lazy callback that supplies the pending exception.
Track whether the access throws and fail before printing "ok" when it does not.
As per coding guidelines, tests must prove their preconditions and must not silently swallow failures.
Proposed fix
- `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`,
+ `globalThis.Symbol = NaN; Bun.inspect(Bun); let sqlThrew = false; try { Bun.sql } catch { sqlThrew = true; } if (!sqlThrew) throw new Error("Bun.sql did not throw"); console.log("ok");`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");`, | |
| `globalThis.Symbol = NaN; Bun.inspect(Bun); let sqlThrew = false; try { Bun.sql } catch { sqlThrew = true; } if (!sqlThrew) throw new Error("Bun.sql did not throw"); console.log("ok");`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/bun/util/inspect.test.js` at line 591, Update the test command around
Bun.sql so it records whether accessing Bun.sql throws after globalThis.Symbol
is set to NaN, and exits or asserts failure before printing "ok" when no
exception occurs. Preserve the success output only when the intended
lazy-property failure is observed, and avoid silently swallowing the catch
result.
Source: Coding guidelines
|
Closing as a duplicate. The bindings.cpp change here is identical to the one in #37107, and #37213 contains both that and the same BunObject.cpp cleanup, plus equivalent hardening for the util.inspect lazy initializers in ZigGlobalObject.cpp. Several fuzzer findings converged on the same root cause (a throwing lazy property callback reports the slot as not found with the exception left pending, and the inspect walk continued without clearing it). Deferring to those earlier PRs. For the record on the open review comments: draining stderr in the test is a fair point, and the same applies to the test in the surviving PRs if it is not already handled there. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); |
There was a problem hiding this comment.
🟡 The test sets stderr: "pipe" but never drains it — Promise.all only reads stdout and awaits proc.exited. Per REVIEW.md's "Subprocess tests: drain pipes concurrently" rule (and the immediately-preceding test at line 573), this should be const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) followed by expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 }).
Extended reasoning...
What the issue is
The new test at inspect.test.js:582-600 spawns a subprocess with stderr: "pipe" (line 595), but the Promise.all on line 597 only awaits proc.stdout.text() and proc.exited — proc.stderr is never read. This violates the repository's explicit harness convention documented in REVIEW.md under 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.
It also diverges from the immediately-preceding test in the same file ("Bun.inspect huge sparse array…", lines 571-579), which follows this pattern exactly and asserts on a combined { stdout, stderr, exitCode } object — so this also runs against Copy harness conventions exactly.
Step-by-step walk-through
- Line 587-596:
Bun.spawnis called withstdout: "pipe"andstderr: "pipe", so both streams are backed by OS pipes that must be drained by the parent. - Line 597:
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);— stdout is drained, but nothing ever callsproc.stderr.text(). - Lines 598-599 assert only on
stdoutandexitCode; stderr is silently discarded.
If the child wrote ≥64KB to stderr, its write(2) would block on the full pipe buffer and the child would never exit, so proc.exited would never resolve and the test would hang until the runner's timeout. In this specific case the child script (globalThis.Symbol = NaN; Bun.inspect(Bun); try { Bun.sql } catch {} console.log("ok");) writes very little (or nothing) to stderr on the success path, so a deadlock is unlikely in practice.
Why this still matters
Beyond the (small) deadlock risk, dropping the stderr assertion loses diagnostic value for exactly the failure mode this test guards against. The bug being fixed is a debug-build assertion abort, and JSC's releaseAssertNoException failure message goes to stderr. If the fix regresses, the test will fail with expect(exitCode).toBe(0) but the assertion output on stderr — the thing that actually explains the failure — is thrown away. Asserting stderr: "" also catches the case where the child prints a warning or partial crash dump but still exits 0.
Why existing code doesn't prevent it
Nothing in the test harness auto-drains piped streams; if you request stderr: "pipe", you own reading it. The await using proc disposal will close the parent's read end after the test body completes, but that happens after await proc.exited, so it can't unblock a child that's stuck writing.
Suggested fix
Match the preceding test's shape:
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "ok\n", stderr: "", exitCode: 0 });Alternatively, if stderr is intentionally uninteresting, drop stderr: "pipe" from the spawn options — but given that the failure mode under test writes its abort message to stderr, draining and asserting on it is the better choice.
1 participant
Fixes a crash found by fuzzing (fingerprint
4d6425ee33c832f1).Repro
Debug builds abort with:
Bun.$andBun.sqlare lazyPropertyCallbackproperties whose first read evaluates an internal module (shell.ts callsSymbol("cwd")at the top level, the sql module chain callsSymbol("resolve")). With the globalSymbolclobbered, that evaluation throws.setUpStaticFunctionSlotthen reports the slot as not found with the exception left pending.Two bugs surfaced from that:
In
JSC__JSValue__forEachPropertyImpl(the walk behindBun.inspectand console formatting), the not-foundcontinueskipped theCLEAR_IF_EXCEPTIONthat followsgetPropertySlot, so the pending exception leaked into the next property's lazy callback and tripped its exception scope assertion. The fix clears the exception before checking the not-found case, matching whatJSC__JSValue__forEachPropertyOrderedalready does.The
Bun.sqllazy callbacks had aBUN_DEBUG-onlyreportUncaughtExceptionAtEventLoopcall on failure. That runs the uncaught exception machinery re-entrantly in the middle of a static property lookup, mutates the Bun object mid-walk, and trips the stale structure assertion inJSObject::getPropertySlot(object->structure() == thisinStructure::storedPrototype). It was reachable with justglobalThis.Symbol = NaN; try { Bun.sql } catch {}. The exception it reported already propagates to the caller (where it can be caught normally), so the call is removed.Release builds compile these assertions out, so this is a debug/ASAN-only crash; behavior there is unchanged apart from no longer running the uncaught exception reporter for a caught error.
Tests
Added a regression test in
test/js/bun/util/inspect.test.jsthat runs the repro in a subprocess. It aborts on a debug build without the fix and passes with it. Also ran the fullinspect.test.js,inspect-error.test.js, andtest/js/bun/consolesuites (the twoinspect-error.test.jssnapshot failures reproduce on an unmodified main build and are unrelated).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