Conversation
…pect JSC__JSValue__forEachPropertyImpl skipped its CLEAR_IF_EXCEPTION when getPropertySlot returned false, but a lazy static property whose builder throws (and a throwing proxy trap) reports the slot as not found while leaving the exception pending. The walk then continued with a stale VM exception, which trips releaseAssertNoException in debug builds. Reproduced by clobbering a global the Bun.$ builtin reads at setup time and then printing the Bun object: globalThis.process = undefined; console.log(Bun); Clear the exception before the continue, matching JSC__JSValue__forEachPropertyOrdered.
|
Warning Review limit reached
Next review available in: 18 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 (1)
WalkthroughProperty enumeration now clears lookup exceptions and skips missing properties. Lazy ChangesInspection resilience
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`:
- Around line 936-940: Add coverage in the test named “console.log survives a
lazy property builder throwing mid-walk” for a separate object whose proxy get
trap throws, then log it and assert execution still prints “SURVIVED” and exits
with code 0. Preserve the existing lazy-property case while exercising the
distinct throwing-proxy path.
- Around line 942-948: Update the Bun.spawn promise collection in this
subprocess test to drain proc.stderr concurrently with proc.stdout.text() and
proc.exited. Preserve the existing stdout and exitCode results while awaiting
all three operations together, ensuring the configured stderr pipe cannot block
the child process.
🪄 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: 39ebccea-9603-4b43-9b65-5e44b2ea4c7f
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/util/inspect.test.js
| it("console.log survives a lazy property builder throwing mid-walk", async () => { | ||
| const code = ` | ||
| globalThis.process = undefined; | ||
| console.log(Bun); | ||
| console.log("SURVIVED"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for the throwing proxy-trap path.
The C++ change handles both failed lazy property builders and throwing proxy traps. This test covers only the lazy Bun property path. Add a proxy get trap that throws and verify that execution still reaches SURVIVED with exit code 0.
As per coding guidelines, tests must cover the complete relevant variant matrix, including distinct error paths. The PR objective also identifies throwing proxy traps as a handled path.
🤖 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 936 - 940, Add coverage in the
test named “console.log survives a lazy property builder throwing mid-walk” for
a separate object whose proxy get trap throws, then log it and assert execution
still prints “SURVIVED” and exits with code 0. Preserve the existing
lazy-property case while exercising the distinct throwing-proxy path.
Source: Coding guidelines
|
Applied the stderr drain in 0d0389001f5, the test now awaits stdout, stderr, and exit together. Skipping the proxy-trap test case: I checked it against the unfixed binary and a throwing get trap does not reach the changed branch. The formatter reads the proxy's target without invoking traps (a proxy with a throwing get trap prints its target's values and exits 0 on builds without this fix), so that case would pass before and after and adds no regression coverage. The reachable case for the fixed line is the lazy static property builder, which the test covers. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The stderr drain landed in 0d03890 before this review ran, the test now awaits stdout, stderr, and exit together. Not asserting stderr is empty on purpose: the child is a debug build and some CI lanes emit harmless allocator or sanitizer notes on stderr, so an exact-empty check is a flake risk. The neighboring ASAN test in this file makes the same tradeoff. A regression still fails clearly here: the child aborts, SURVIVED never prints, and the exit code check fails. |
There was a problem hiding this comment.
LGTM — my earlier stderr-drain comment was stale on arrival; 0d03890 already fixed it and the author's rationale for not asserting stderr === "" (debug-build noise on some CI lanes) is sound.
What was reviewed:
- The reorder in
forEachPropertyImplnow matchesforEachPropertyOrdered(bindings.cpp:5763-5765) — clear-then-check instead of check-then-clear. - Confirmed the no-exception path is behaviorally unchanged; only the throw-and-return-false case now clears before
continue. - Test drains all three pipes concurrently; the declined proxy-trap coverage was verified by the author to not reach the changed branch.
Extended reasoning...
Overview
Two-file change: a 5-line reorder in src/jsc/bindings/bindings.cpp (JSC__JSValue__forEachPropertyImpl) so CLEAR_IF_EXCEPTION(scope) runs before the if (!hasProperty) continue; check, and a subprocess regression test in test/js/bun/util/inspect.test.js. The fix addresses a Fuzzilli-found debug-build abort where a lazy static property builder (Bun.$) throws during getPropertySlot, JSC returns false with a pending exception, and the old code continued past the clear, tripping releaseAssertNoException on the next JSC entry.
Security risks
None. This is exception-scope hygiene in the inspect/formatter path — no auth, crypto, permissions, or untrusted-input parsing. The change strictly narrows the window during which a pending exception can persist.
Level of scrutiny
Low. The change is mechanical: store the bool, clear, then branch — exactly what the sibling JSC__JSValue__forEachPropertyOrdered already does at bindings.cpp:5763-5765. The no-exception path is byte-for-byte equivalent to the old behavior. The PR description traces the mechanism from repro through JSC's setUpStaticFunctionSlot to the assertion, and the author verified the test aborts on an unfixed debug build.
Other factors
All prior review feedback is resolved: the stderr-drain concern (raised by both CodeRabbit and my earlier run) landed in 0d03890 before my comment posted, and the current diff confirms proc.stderr.text() is awaited in the Promise.all. The author declined the proxy-trap test suggestion after verifying it doesn't reach the changed branch on an unfixed binary — a valid "test would pass before and after" rejection per REVIEW.md. The choice not to assert empty stderr matches the neighboring ASAN test's tradeoff and avoids flakes from allocator/sanitizer chatter.
|
Updated 5:37 PM PT - Aug 9th, 2026
❌ @robobun, your commit c4ad31e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37292That installs a local version of the PR into your bun-37292 --bun |
…rows Requiring node:util runs the module body, which throws when user code has clobbered globals it reads (globalThis.process = undefined). The lazy property initializers for m_utilInspectFunction and m_utilInspectStylizeColorFunction returned early on exception without calling init.set(), violating the LazyProperty contract: debug builds hit the lazyTag assertion and release builds fail fast. Reachable from plain JS on every platform by formatting any object with a nodejs.util.inspect.custom function after clobbering process. On Windows console.log(Bun) reaches it through Bun.env, whose custom inspect loads util.inspect. Clear the non-termination exception and install a stub inspect function (or the no-color stylizer) so formatting degrades instead of aborting.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Fallback for when requiring node:util throws during lazy initialization of | ||
| // m_utilInspectFunction (e.g. globalThis.process was clobbered by user code). |
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
| // Requiring node:util runs JS that can throw (e.g. user code clobbered | ||
| // globalThis.process). The initializer must still set the property, so | ||
| // degrade to a stub instead of crashing. |
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
| // The initializer must still set the property even when util.inspect is | ||
| // unavailable; fall back to the stylizer that applies no colors. |
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
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/bindings/ZigGlobalObject.cpp:2352-2372— The no-color fallback at lines 2369-2372 is unreachable for the scenario it targets:m_utilInspectFunctionnow always clears its exception and installs a value (real inspect or the stub), soscope.exception()at 2354 is never set, andgetStylizeWithColoris a pure factory that returns a closure without touching its argument — so calling it with the stub succeeds andinit.set(stylize)at 2364 always runs. What actually gets cached is astylizeWithColorclosure whose capturedinspectis the bare host stub with no.styles/.colors; the firstoptions.stylize(str, type)call with colors on hitsundefined[styleType]and throws. Either check for the stub's identity before calling the factory and take the no-color path there, or attach empty.styles/.colorsto the stub.Extended reasoning...
What the bug is
The
m_utilInspectStylizeColorFunctioninitializer adds a fallback branch (lines 2369-2372) whose comment says "fall back to the stylizer that applies no colors" whenutil.inspectis unavailable. That branch is dead for the scenario this PR targets, and instead of the no-color stylizer, a broken stylizer closure gets cached — one that throws a TypeError the first time it's actually invoked.Why the fallback is unreachable
Trace the initializer at
ZigGlobalObject.cpp:2348-2373after this PR's changes:-
Line 2352 calls
utilInspectFunction(). The reworkedm_utilInspectFunctioninitializer (lines 2314-2332) now handles the "requiring node:util threw" case by callingscope.tryClearException()and theninit.set(<stub>). It always installs a value and never leaves an ordinary exception pending. So after line 2352,scope.exception()is unset. -
Line 2354
if (!scope.exception())is therefore always entered. -
Line 2359 calls
getStylizeWithColor(inspect)viaprofiledCall. Look atsrc/js/builtins/UtilInspect.ts:4-15:export function getStylizeWithColor(inspect: Inspect) { return function stylizeWithColor(str, styleType) { const style = inspect.styles[styleType]; ... }; }
This is a pure factory — it returns a closure without dereferencing
inspectat all. Passing the host stubjsFunctionUtilInspectFallback(a plain native JSFunction) succeeds with no exception;returnedExceptionstays null. -
Line 2363
dynamicDowncast<JSFunction>(result)— the returned closure is a JSFunction, so this succeeds,init.set(stylize)runs at line 2364, and the initializer returns.
Lines 2369-2372 are never reached. (Minor caveat: a TerminationException would survive
tryClearException()and could reach the fallback, but that isn't the case this PR is about.)What actually gets cached
A
stylizeWithColorclosure whose capturedinspectis the bare host functionjsFunctionUtilInspectFallback. That stub has no.stylesor.colorsproperties — it's justJSFunction::create(vm, owner, 2, "inspect"_s, jsFunctionUtilInspectFallback, ...)with nothing attached.Step-by-step proof of the failure mode
Concrete sequence with
globalThis.process = undefinedand colors enabled:- User code clobbers
process, then formats an object with a custom inspect that callsoptions.stylize("foo", "string"), on a TTY (or withFORCE_COLOR) socolors=true. UtilInspect.cppselectsutilInspectStylizeColorFunction()because colors is true.- That triggers the
m_utilInspectStylizeColorFunctioninitializer, which first triggers them_utilInspectFunctioninitializer. - Requiring
node:utilthrows (process is undefined). The initializer clears the exception and installs the stub. - Back in the color-stylizer initializer,
scope.exception()is clear → factory is called with the stub → closure is returned → cached. - The custom inspect callback runs and calls
options.stylize("foo", "string"). - Inside the closure:
inspect.styles[styleType]→ the stub has no.styles→undefined["string"]→ TypeError: undefined is not an object.
The comment's promised behavior — "fall back to the stylizer that applies no colors" — never happens.
Why the new tests don't catch this
Both new tests spawn with
stdout: "pipe", so stdout is not a TTY, colors defaults to false, andutilInspectStylizeNoColorFunction()is selected instead of the color one (UtilInspect.cpp:30). The second test's custom inspect also never callsoptions.stylize. Neither exercises the cached broken closure.Impact
This is fuzzer-territory niche (user has already clobbered
globalThis.process), and pre-PR the same scenario was a hard SIGABRT, so this PR is still a strict improvement. The stylize TypeError would also be caught by the exception handling around the custom-inspect call rather than aborting. But per REVIEW.md — "Every line you add must be demonstrably live" — this is four lines of dead code with a comment that describes behavior the code doesn't have, and the value that is cached is broken.How to fix
Simplest: after calling
utilInspectFunction(), check whether the returned function is the stub (e.g. compare its native function pointer, or set a flag on the global when installing the stub) and go straight to the no-color fallback if so — before calling the factory. Alternatively, attach emptystyles/colorsobjects to the stub in them_utilInspectFunctioninitializer so the cached closure degrades to a no-op instead of throwing. Either way the dead branch and its comment can then be removed or actually wired. -
getStylizeWithColor returns a closure without reading its argument, so the color stylizer initializer happily wrapped the fallback inspect stub and the closure then threw on first stylize call (the stub has no styles or colors). Check for inspect.styles before building the color stylizer so the broken environment gets stylizeWithNoColor instead.
|
Good catch on the stylizer. getStylizeWithColor never dereferences its argument, so the color path cached a closure around the stub and the first options.stylize call with colors on threw (verified with FORCE_COLOR=1, the child died with a TypeError from stylizeWithColor and never reached the end of the script). Fixed in 5c28744 by checking for inspect.styles before building the color stylizer, which is the capability the closure actually needs at call time. The stub has no styles, so the broken environment now takes the colorless stylizer, and the no-color fallback branch is reachable exactly when it should be. Verified both directions: FORCE_COLOR=1 with process clobbered prints unstyled output and exits 0, and a healthy environment still emits ANSI codes. Added a regression test for the colors case. |
| // stylizeWithColor reads inspect.styles at call time; the fallback stub | ||
| // installed when node:util fails to load has none, so stay colorless then. |
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
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 990-992: Strengthen the assertions in this test by replacing the
separate stdout containment checks with one exact equality assertion against
"hi\nSURVIVED\n". Keep the existing exitCode assertion unchanged so the test
verifies both colorless output and successful execution.
🪄 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: 3f1eb219-eb8d-4046-9c29-195c68dfb27d
📒 Files selected for processing (2)
src/jsc/bindings/ZigGlobalObject.cpptest/js/bun/util/inspect.test.js
|
This one arrived against an older sha. The stylizer gap it describes was the same finding from the previous review round and was fixed in 5c28744: the initializer now checks for inspect.styles before building the color stylizer (the suggested option 2), so the stub routes to stylizeWithNoColor and the fallback branch is live. The piped-stdout concern is covered too: the regression test added in that commit sets FORCE_COLOR=1, which enables colors under a pipe, and it failed with the stylize TypeError before the fix. |
There was a problem hiding this comment.
I reviewed this PR again after the stylizer fix in 5c28744 and the follow-up test tightening — no bugs found this round. Because it rewrites two LazyProperty initializers in ZigGlobalObject.cpp (exception-scope handling, tryClearException, fallback host functions), a maintainer look would still be worthwhile.
What was reviewed:
bindings.cppreorder now matchesforEachPropertyOrdered(bindings.cpp:5763-5767) exactly.- The
inspect.stylesgate makes the no-color fallback reachable for the stub;getStylizeWithColorno longer captures a styles-less function. tryClearException()leaves termination exceptions pending, andinit.set()is now reached on every path in both initializers.- Tests drain stderr concurrently and the FORCE_COLOR case asserts exact colorless output.
Extended reasoning...
Overview
Three files: a 5-line reorder in src/jsc/bindings/bindings.cpp (forEachPropertyImpl now clears the exception before the !hasProperty continue, matching its ordered sibling), a ~22-net-line rewrite of two LazyProperty initializers in src/jsc/bindings/ZigGlobalObject.cpp (m_utilInspectFunction and m_utilInspectStylizeColorFunction) so they always call init.set() even when requiring node:util throws, plus a new 5-line host-function stub. Three subprocess regression tests in test/js/bun/util/inspect.test.js.
Security risks
None identified. The change swallows exceptions via tryClearException(), which by design leaves termination exceptions pending, so this does not create a way to survive a VM termination. The fallback stub is only installed when node:util itself fails to load — a deliberately broken environment. No new user-controlled input is parsed.
Level of scrutiny
Moderate-to-high. The bindings.cpp reorder is mechanical and mirrors the existing ordered variant line-for-line. The ZigGlobalObject.cpp change is more substantial: it replaces RETURN_IF_EXCEPTION early returns (which violated the LazyProperty must-always-set contract) with nested if (!scope.exception()) success paths and a shared fallback tail. This is JSC exception-scope C++ in a file that ships to every user, and this PR already went through one round where an earlier version installed a broken color stylizer (fixed in 5c28744 by gating on inspect.styles). That history argues for a human pass rather than auto-approval.
Other factors
- All prior review feedback (mine and CodeRabbit's) has been addressed: stderr is drained concurrently, the FORCE_COLOR test asserts exact stdout, and the stylizer gate is live. The one declined nit (proxy-trap coverage) was declined with a checked reason — the formatter reads proxy targets without invoking traps, so that path does not reach the changed line.
- The comment-cop bot fired on earlier commits; the current diff has single-line comments only.
dynamicDowncastreplacesuncheckedDowncaston the happy path, which is strictly safer.- I did not find any other
LazyPropertyinitializers in this file with the sameRETURN_IF_EXCEPTION-before-init.set()shape that this PR leaves unfixed for the same trigger, but I did not exhaustively audit every initializer — worth a quick scan by someone who knows which ones can enter user JS.
|
On the open question about other initializers with the same shape: scanned every initLater lambda in src/jsc/bindings. m_utilInspectFunction is the only one that requires an internal module, and m_utilInspectStylizeColorFunction (calling the getStylize builtin) is the only other one that runs JS. Both are the ones fixed here; the remaining RETURN_IF_EXCEPTION uses in ZigGlobalObject.cpp are in ordinary member functions, not lazy property initializers. |
|
Consolidated into #37160, which carries the same |
1 participant
What does this PR do?
Fixes two crashes surfaced by a Fuzzilli-found abort (fingerprint
9615b92f1ed60a34):Deterministic reproduction for both:
Fix 1: pending exception leak in the inspect property walk (bindings.cpp)
console.log(Bun)walks properties viaJSC__JSValue__forEachPropertyImpl. The Bun object has non-reified static properties, so the walk takes the slow path and callsgetPropertySlotper property.$runs the lazy builderconstructBunShell, which executes the shell builtin's top level. That code readsprocess.env, so withprocessclobbered it throws a TypeError. JSC'ssetUpStaticFunctionSlotdeliberately reports the slot as not found and leaves the exception pending for the caller to handle (Lookup.cpp).if (!getPropertySlot) continue;before itsCLEAR_IF_EXCEPTION, so that pending exception was never cleared. The next re-entry into JSC tripsreleaseAssertNoExceptionand the process aborts on debug builds. The ordered variant (JSC__JSValue__forEachPropertyOrdered) already clears before checking the result; only the non-ordered one had the two statements swapped. The fix reorders them to match.About the misleading message text: the TypeError comes from
process.env, but errors raised while a builtin function frame is on top get attributed to bytecode index 0 of that builtin (getBytecodeIndexskips builtin frames' real index), and the first expression info entry of the shell builtin is thecreateShellInterpreterbinding. That is why the fuzzer's crash names a function the crashing script never used. The fuzzer hit this flakily because it needs one REPRL iteration to clobber a global and a later one to inspectBun.Fix 2: LazyProperty contract violation in the util.inspect initializers (ZigGlobalObject.cpp)
The same repro crashed the Windows CI lanes through a second path. Formatting a value with a
nodejs.util.inspect.customfunction (on Windows,console.log(Bun)reaches this throughBun.env) lazily initializesm_utilInspectFunction, whose initializer requiresnode:util. The module body throws withprocessclobbered, and the initializer returned early without callinginit.set(). A LazyProperty initializer must always set the property: debug builds hitASSERTION FAILED: !(initializer.property.m_pointer & lazyTag)and release builds fail fast (0xC0000409 on Windows; also reproducible on Linux release). The fix clears the non-termination exception and installs a stub inspect function, and does the same form_utilInspectStylizeColorFunctionwith the no-color stylizer, so formatting degrades instead of aborting. Both paths are reachable from plain user JS on every platform.How did you verify your code works?
bun-debug -e 'globalThis.process = undefined; console.log(Bun)'aborts with the exact assertion above on Linux, and with the lazyTag assertion on Windows; release builds crash on the custom-inspect path on both platforms. After the fixes it prints the Bun object and exits 0 on both.test/js/bun/util/inspect.test.js(one per fix). Each fails on an unfixed debug build (child aborts) and passes with this change.bun bd test test/js/bun/util/inspect.test.jsis green on Linux and Windows, andtest/js/bun/console/andtest/js/node/util/are green on Linux (two pre-existing stress-test timeouts also occur on an unmodified build).util.inspectstill reaches custom inspect functions whennode:utilloads fine.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/inspect.test.js