Conversation
…uring enumeration JSC__JSValue__forEachPropertyImpl kept iterating with the exception still pending when getPropertySlot returned false after a throw (a throwing static lazy property builder or a proxy trap). Debug builds then aborted with "Unexpected exception observed" at the next host call, and release builds silently dropped every property that came after the throwing one. Clear the exception before skipping the property, the same way JSC__JSValue__forEachPropertyOrdered already does.
WalkthroughThe property enumeration binding now records lookup results before clearing pending exceptions. A regression test verifies that inspection continues after a throwing lazy getter. ChangesProperty inspection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:51 AM PT - Aug 7th, 2026
✅ @robobun, your commit cdee1242a69cc05b47dab83bb78a881559b6df1c passed in 🧪 To try this PR locally: bunx bun-pr 37107That installs a local version of the PR into your bun-37107 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the reorder so CLEAR_IF_EXCEPTION runs before the continue is the obvious fix and matches how forEachPropertyOrdered already handles the same call. One inline nit on the test's stderr drain, not blocking.
Extended reasoning...
Overview
This PR fixes a control-flow bug in JSC__JSValue__forEachPropertyImpl (src/jsc/bindings/bindings.cpp) where getPropertySlot returning false with a pending exception would continue past the CLEAR_IF_EXCEPTION call, leaving a stale exception on the VM for the next iteration. The fix stores the return value, clears the exception unconditionally, then checks — a 3-line reorder. A subprocess regression test is added to test/js/bun/util/inspect.test.js.
Security risks
None. This is exception-scope hygiene in the inspect/console-formatting property walk; no auth, crypto, or untrusted-input parsing involved.
Level of scrutiny
Low-to-medium. The change is mechanical and mirrors the exact pattern already used by the sibling JSC__JSValue__forEachPropertyOrdered at bindings.cpp:5547-5551 (bool hasProperty = ...; scope.tryClearException(); if (!hasProperty) continue;). The PR description gives a precise root-cause trace (throwing lazy static property builder → setUpStaticFunctionSlot returns false with exception pending → continue skips clear → next host call's releaseAssertNoException aborts), and the fix is the minimal correction for that trace.
Other factors
- Checked the other
getPropertySlotcall sites in this file: the one at :4389 already hasRETURN_IF_EXCEPTIONon both branches, andforEachPropertyOrderedalready had the correct ordering — so this was the only site with the bug. - The regression test spawns a child (correct, since it clobbers
globalThis.process) and asserts properties enumerated after the throwing getter still appear. The PR states it fails on release (false true) and aborts on unfixed debug, which is the right failure shape. - One nit was filed inline: the test pipes stderr but doesn't drain it. On the fixed path stderr is empty so there's no deadlock risk; it's a convention/diagnostics improvement, not a correctness issue.
- The updated comment accurately documents the second failure mode (throwing static lazy property builders) beyond the original proxy-trap note.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Ignore exceptions from "Get" proxy traps and throwing static | ||
| // lazy property builders (which return false with an exception | ||
| // pending, see setUpStaticFunctionSlot). |
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
| // Ignore exceptions from "Get" proxy traps and throwing lazy | ||
| // property builders; both return false with the exception pending. |
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
![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 815-816: Update the assertions around Bun.inspect(Bun) to verify
the formatted property entries for argv and gc, rather than checking arbitrary
substring presence. Ensure the test confirms both property labels are actually
listed in the inspection output and preserves the objective of checking later
properties remain included.
🪄 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: 22369d78-ce4e-46fa-a0d3-3721e836ffc7
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/util/inspect.test.js
|
Tightened the test to match the formatted property entries (argv: [ and gc: [Function: gc]) instead of bare substrings, in dc55c09. Verified it still fails on the current release build and passes with the fix. |
There was a problem hiding this comment.
LGTM — the reordering now matches forEachPropertyOrdered a few lines down (store result → clear → check), and all earlier review feedback has been addressed. The remaining inline nit is about future test-rot resilience, not correctness today.
Extended reasoning...
Overview
Two files touched: a 5-line reordering in src/jsc/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl, and a new subprocess regression test in test/js/bun/util/inspect.test.js.
The C++ change moves CLEAR_IF_EXCEPTION(scope) to run before the continue, so a throwing getPropertySlot (proxy trap or lazy static-property builder) no longer leaves a pending exception on the VM while iteration continues. I verified this now byte-for-byte matches the sibling JSC__JSValue__forEachPropertyOrdered at bindings.cpp:5544-5549 (bool hasProperty = ...; scope.tryClearException(); if (!hasProperty) continue;), which was the PR's stated reference.
Security risks
None. This is exception-scope hygiene in a diagnostic/formatting path (Bun.inspect / console). No user-controlled input reaches new code paths; the change only ensures an already-intended CLEAR_IF_EXCEPTION runs on the false-return branch it was skipping.
Level of scrutiny
Low-to-medium. It's C++ in JSC bindings, but the transform is mechanical (store result, clear, then branch) and mirrors existing code in the same file. The root-cause explanation in the PR description is precise and I confirmed it against the surrounding code.
Other factors
- Prior feedback addressed: my earlier stderr-drain nit (78f2e47), CodeRabbit's substring-match nit (dc55c09), and the comment-cop length complaint (d5c72a7 — comment is now one line) are all resolved in the current diff.
- Test quality: follows the sibling "huge sparse array" test's exact pattern — three-way
Promise.all, combined{stdout, stderr, exitCode}assertion,await using. Author reports it fails on the current release build (false true) and aborts the unfixed debug build. - Remaining nit (posted inline): the test's trigger relies on
Bun.$'s lazy builder throwing whenprocessis clobbered, which isn't a stable contract. It's a valid rot-resilience suggestion but the test is correct as-shipped, so not blocking.
…erties The trigger (the lazy Bun.$ builder throwing when process is clobbered) is an implementation detail, so assert it held: $ must be absent from the output. Also check version and semver so the release-mode signal does not hinge on where argv lands in enumeration order.
There was a problem hiding this comment.
LGTM — the remaining inline nit (stale "third check" ordinal in the test comment) is cosmetic and non-blocking.
Earlier feedback on this PR has all been addressed: stderr is now drained and asserted in the combined object, the C++ comment was collapsed to one line, the substring checks were tightened to formatted property entries, and the test now asserts the throwing-getter precondition (!s.includes("$: [Function")) so it fails loudly if the trigger ever disappears.
Extended reasoning...
Overview
Four-line reorder in src/jsc/bindings/bindings.cpp (JSC__JSValue__forEachPropertyImpl): store the getPropertySlot result in a local, run CLEAR_IF_EXCEPTION(scope) unconditionally, then continue if the lookup failed. Previously the continue fired before the clear, leaving a pending exception on the VM that aborted debug builds at the next releaseAssertNoException and made release builds silently drop every subsequent static property. A regression test in test/js/bun/util/inspect.test.js spawns a child that clobbers process (making the lazy Bun.$ builder throw during enumeration) and asserts that later properties still appear, that $ was skipped, and that stderr/exitCode are clean.
Security risks
None. This is exception-scope hygiene in the console-formatting property walk; no auth, crypto, untrusted-input parsing, or new API surface.
Level of scrutiny
Low-to-medium. The change is mechanical and mirrors the identical pattern already used in the sibling JSC__JSValue__forEachPropertyOrdered a few lines down (bool hasProperty = ...; (void)scope.tryClearException(); if (!hasProperty) continue;). REVIEW.md's "never clearException()" rule is about swallowing errors on the success path — here the intent has always been to ignore getter exceptions during inspect enumeration (the pre-existing comment said so); the fix just makes the clear actually run on the false-return path too.
Other factors
- The PR description includes stamp-gate evidence: the new test fails on both the unfixed debug (abort) and release (
false true) builds and passes with the fix. - All three rounds of prior review feedback (mine and CodeRabbit's) were addressed in follow-up commits: stderr draining + combined-object assertion (78f2e47), C++ comment trimmed to one line (031e281 / d5c72a7), formatted-entry substring matches (dc55c09), and the precondition assertion + extra surviving-property checks (0f1c4bc).
- The comment-cop bot's two firings were both on the earlier multi-line C++ comment, which is now a single line.
- The one remaining finding this run is a stale ordinal in the test's block comment ("the third check" now refers to the fifth argument after 0f1c4bc inserted two more). It misdirects a reader slightly but has no functional effect.
On Windows, process.env is a Proxy whose target carries a custom inspect symbol, and rendering it lazily requires node:util, whose top level reads the global process binding. With process clobbered that require throws inside the utilInspectFunction LazyProperty initializer and the child aborts (reported separately). Clobbering Promise keeps process intact while still making the lazy Bun.$ getter throw, since ShellPromise extends the global Promise binding.
|
The Windows CI failures were a separate crash the test happened to expose: on Windows, process.env is a Proxy whose target carries a nodejs.util.inspect.custom function, and rendering that lazily requires node:util, whose top level reads the global process binding. With process clobbered, the require throws inside the utilInspectFunction LazyProperty initializer, which then aborts in LazyPropertyInlines.h (release builds fast-fail with 0xC0000409, which is the exit code 9 CI saw). That bug is reported separately with a cross-platform repro. cdee124 switches the test's trigger to clobbering Promise instead: ShellPromise extends the global Promise binding, so the lazy Bun.$ getter still throws during reification, while process stays intact and the env custom-inspect path stays healthy. Verified on Linux and Windows x64 debug builds (74/74 in the file on both), and the comment's stale ordinal is fixed along the way. |
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the fix now mirrors forEachPropertyOrdered exactly and the test guards its own trigger.
Checked in the final revision:
- The reordered
CLEAR_IF_EXCEPTIONmatches the sibling atbindings.cpp:5544-5549(forEachPropertyOrdered), so both property walks now handle a throwinggetPropertySlotidentically. - Test drains stderr, asserts formatted property labels, and includes the
!s.includes("$: [Function")precondition so it fails loudly if the shell-builtin trigger ever stops throwing. - Switching the trigger to
Promisesidesteps the unrelated Windowsprocess.envcustom-inspect LazyProperty abort the earlier revision hit.
Extended reasoning...
Overview
Two-file change: a 4-line reorder in src/jsc/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl, plus a subprocess regression test in test/js/bun/util/inspect.test.js. The old code did if (!getPropertySlot(...)) continue; followed by CLEAR_IF_EXCEPTION, so a throwing lazy property builder or proxy trap left the exception pending across the continue. Debug builds aborted at the next host-call releaseAssertNoException; release builds silently dropped every subsequent static property. The fix stores the result in a local, clears the exception, then checks the local — the same shape JSC__JSValue__forEachPropertyOrdered already uses a few hundred lines below.
Security risks
None. This is inspect/console-formatting code; the change moves an existing exception clear rather than introducing one, and does not touch any auth, network, filesystem, or user-input parsing path.
Level of scrutiny
Low-to-moderate. It's native JSC bindings code, but the change is mechanical and copies an established sibling pattern verbatim. REVIEW.md's "Never clearException()" rule is about not swallowing errors in general control flow — this specific inspect path already deliberately clears getter exceptions (both before this PR and in the ordered sibling) because inspect is best-effort formatting; the PR only fixes when the existing clear happens so it also covers the false-return path.
Other factors
This PR has been through three rounds of my own review feedback plus CodeRabbit and comment-cop, all of which are now addressed in the head commit: stderr is drained and asserted in the combined {stdout, stderr, exitCode} object; property labels are matched instead of bare substrings; the $-skipped precondition is asserted so the test can't rot silently; the C++ comment is one line; the test-comment ordinal is order-independent ("the last check"); and the trigger was switched from clobbering process to clobbering Promise after the earlier revision exposed an unrelated Windows-only LazyProperty abort in the process.env custom-inspect path (reported separately per the author). The PR description includes gate evidence showing the test fails on the unfixed debug build and passes with the fix, and the author confirmed 74/74 on both Linux and Windows x64 debug after the trigger switch.
|
Another fuzzer fingerprint (6d6017f820815681) lands in this same enumeration path from a different entry point: Symbol++; // globalThis.Symbol becomes NaN
new CompressionStream(globalThis);The constructor rejects the argument with ERR_INVALID_ARG_VALUE, and formatting that message inspects the received value, so enumeration walks globalThis and Bun and runs the throwing lazy initializers (shell.ts calls On this branch (cdee124) that abort is gone, but the repro still aborts in debug builds, and so does the direct form: globalThis.Symbol = NaN;
Bun.inspect(Bun); // fine with Promise clobbered instead, aborts with Symbol clobberedThe inline walk in Two observations that localize it: a single #37160 avoids this by resolving the slot with per-level fresh prototype reads ( |
|
Consolidated into #37160, which clears the exception the same way and additionally replaces the prototype walk so the |
1 participant
Fixes a crash found by fuzzing (fingerprint
a30423e4170ed1d6): debug builds aborted withRoot cause
JSC__JSValue__forEachPropertyImpl(the property walk behindBun.inspectand console formatting) fetches each property withgetPropertySlot. When that lookup throws, it returns false with the exception pending; this happens for throwing proxy traps and for static lazy property builders that enter JS, persetUpStaticFunctionSlot. The code did:so the
continueskipped the clear, and iteration kept running with a stale exception on the VM. On debug builds the next lazy getter's host-call validation scope hitreleaseAssertNoExceptionand aborted. On release builds the stale exception madesetUpStaticFunctionSlotreport every later static property as not found, soBun.inspect(Bun)silently dropped most of the object once one getter threw.The fuzzer hit this through REPRL state: an earlier iteration clobbered a global (
Symbol,Object, orprocess), thenBun.inspect(this)enumeratedBun, the lazy$getter evaluated the shell builtin, and the builtin threw (thecreateShellInterpretertext in the message is just the snippet at the start of the combined builtin source). Deterministic repro:Fix
Clear the exception before skipping the property, the same way
JSC__JSValue__forEachPropertyOrderedalready does a few lines down.Test
test/js/bun/util/inspect.test.jsspawns a child that clobbersprocessand checksBun.inspect(Bun)still lists properties that enumerate after the throwing getter. Fails on the current release build (false true) and aborted the unfixed debug build; passes with the fix.While investigating I found a separate debug assert (
Structure::storedPrototypestructure mismatch) reachable withglobalThis.Symbol = undefined; Bun.sqlon current main, unrelated to this change; reported separately.[stamp-90s] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file