Fix assertion failure when inspecting an object whose lazy property initializer throws - #37234
Fix assertion failure when inspecting an object whose lazy property initializer throws#37234robobun wants to merge 2 commits into
Conversation
A static-table PropertyCallback that runs JS (Bun.$, Bun.sql) can throw, for example when the global Symbol has been overwritten. JSC reports the slot as not found with the exception left pending, but the property walk in JSC__JSValue__forEachPropertyImpl skipped its exception clear on that branch, so the stale exception aborted assert-enabled builds at the next property's reification and could leak into unrelated code in release builds. Also remove the debug-only uncaught exception report from the Bun.sql lazy getters: it re-entered the uncaught exception machinery with the exception still pending, tripping a stale-structure assertion inside process.get. The exception already propagates to the caller.
|
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 (1)
WalkthroughChangesLazy property exception handling
Possibly related PRs
🚥 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: 1
🤖 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`:
- Around line 944-950: Update both subprocess test sites in
test/js/bun/util/inspect.test.js at lines 944-950 and 972-978 to consume
proc.stderr.text() concurrently with proc.stdout.text() and proc.exited in the
existing Promise.all calls. Apply the same change to both sites so every
Bun.spawn invocation drains stdout, stderr, and process completion without
altering the test assertions.
🪄 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: b99d307c-b14c-47fe-9989-f0538cd25db3
📒 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
|
Good catch, both children do write diagnostics to stderr in debug builds (reported module eval errors with code frames), so an undrained pipe could block them. Both tests now read stderr in the same Promise.all as stdout and exit. Pushed in ae03357. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Ignore exceptions from "Get" proxy traps and throwing lazy | ||
| // property initializers (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
|
Updated 2:07 AM PT - Aug 9th, 2026
⏳ @robobun, your commit ae03357 is still building in
|
|
On the bindings.cpp comment note: that two line comment is not justifying a workaround, it records the JSC contract that makes the ordering matter (a throwing lazy property initializer reports the slot as not found while the exception stays pending, see setUpStaticFunctionSlot in Lookup.cpp). Without it the clear looks safe to move back below the continue, which is the exact bug this PR fixes. It also follows the same one line style already used in this function for getters and proxy traps, so I left it in place. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This finding was generated against the first commit. Both tests already drain stderr in the same Promise.all since ae03357, which addressed the identical CodeRabbit comment. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Confirmed, this is a duplicate. The property walk fix here is identical to the one in #37107 (open since Aug 7, CI green) and #37213, and the Bun.sql debug report removal is already in #37160, #37175, and #37213. Several fuzzing sessions hit the same root cause through different crash signatures. Closing in favor of the earlier PRs. One note for whichever lands: this signature reached the walk through error message construction, Symbol++ followed by new CompressionStream(globalThis) renders the received value with the inspector and aborted the same way, so that path is worth a regression test if not already covered. |
There was a problem hiding this comment.
LGTM. My earlier inline comment about draining stderr was stale — it was posted against the first commit and ae03357 already fixed both sites.
What was reviewed
- The
CLEAR_IF_EXCEPTIONreorder in the slow path now matches the fast path at bindings.cpp:5532-5536, which already clears before checking the result. - Removed
BUN_DEBUGblocks indefaultBunSQLObject/constructBunSQLObject— verifiedRETURN_IF_EXCEPTIONimmediately after already propagates the error, and re-entering JS viareportUncaughtExceptionAtEventLoopwith a pending exception is the actual violation. - Checked for sibling
getPropertySlot-then-continuesites inforEachPropertyImpl— the fast path is already correct, no others found. - Both regression tests now drain stderr concurrently and use process-isolated fixtures per harness convention.
Extended reasoning...
Overview
This PR fixes a debug-build assertion failure (SIGABRT) when Bun.inspect / console.log / error-message rendering walks an object whose lazy PropertyCallback initializer throws. Three files changed: a two-line reorder in bindings.cpp (JSC__JSValue__forEachPropertyImpl slow path), removal of two 3-line #if BUN_DEBUG blocks in BunObject.cpp, and two subprocess regression tests in inspect.test.js.
Security risks
None. This is exception-scope hygiene in the inspector's property walk. No user-controlled input parsing, auth, crypto, or trust boundaries are touched. The only observable behavior change is that inspect output omits properties whose lazy initializer threw instead of leaving a pending exception (or aborting).
Level of scrutiny
Medium — forEachPropertyImpl is a hot path (every console.log/inspect/ERR_INVALID_ARG_VALUE render), so a wrong change here would be very visible. But the fix is mechanical: it hoists CLEAR_IF_EXCEPTION(scope) above the if (!found) continue, which is exactly the ordering the fast-path branch of the same function already uses (bindings.cpp:5528-5536). The PR description correctly cites the JSC contract (setUpStaticFunctionSlot reports not-found while leaving the exception pending), and the two-line code comment records that so a future refactor doesn't reintroduce the bug. The BUN_DEBUG block removal is debug-only, and calling reportUncaughtExceptionAtEventLoop (which re-enters JS via process.get) while an exception is pending is itself a JSC exception-check violation — the RETURN_IF_EXCEPTION on the next line already does the right thing.
Other factors
All prior review feedback is resolved: CodeRabbit's stderr-draining note and my own stale duplicate of it were both addressed in ae03357 (verified in the current diff — both tests now Promise.all([stdout.text(), stderr.text(), exited])). The github-actions comment-cop flagged the new code comment; the author's response is reasonable — it's a two-line contract note in the same style as the neighboring "Ignore exceptions from Proxy getPrototype trap" comments, not a workaround justification. The regression tests are process-isolated (they clobber the global Symbol), assert signalCode === null for the abort case, and follow file-local subprocess conventions.
1 participant
Fixes a deterministic abort found by fuzzing (fingerprint
a970dd8104b66865).Repro
or, through error message construction:
On assert-enabled builds both abort with:
What happens
Bun.$andBun.sqlare static-tablePropertyCallbackentries whose initializers run JS (shell.ts, sql.ts). When the globalSymbolhas been overwritten, those initializers throw. JSC'ssetUpStaticFunctionSlothandles that by reporting the slot as not found with the exception left pending, and callers are expected to check.The property walk in
JSC__JSValue__forEachPropertyImpl(used byBun.inspect,console.log, and node-style error messages that render the received value) did this:so the pending exception survived into the next property's reification, which aborts debug builds and can surface the stale exception from unrelated code in release builds. The fix hoists the clear above the
continue, matching how the walk already treats throwing getters and proxy traps: the property is skipped.With that fixed, the same repro reached a second assert: the
Bun.sqllazy getters had a debug-only block that calledreportUncaughtExceptionAtEventLoopwhile the module evaluation exception was still pending. That re-entersprocess.getand JS with a pending exception, which trips a stale-structure assertion inJSObject::getPropertySlot(reification transitions the structure, then the pre-existing pending exception makes the lookup report not-found, and the cachedStructure*no longer matches). TheRETURN_IF_EXCEPTIONright after already propagates the error to the caller, so the block is removed.After the fix the repros run to completion: inspect output simply omits the properties whose initializers threw, and direct access like
Bun.$still throws the initializer's error to the caller as before.Tests
Added two regression tests in
test/js/bun/util/inspect.test.js. Both abort the unfixed debug build (SIGABRT) and pass with the fix. The assertion only exists in assert-enabled builds, so they pass on release builds either way.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