console: guard JSX/Proxy recursion in Bun.inspect (stack overflow SIGSEGV) - #29709
console: guard JSX/Proxy recursion in Bun.inspect (stack overflow SIGSEGV)#29709robobun wants to merge 3 commits into
Conversation
|
Updated 9:50 AM PT - Aug 13th, 2026
❌ @robobun, your commit 02da7db has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29709That installs a local version of the PR into your bun-29709 --bun |
|
Warning Review limit reached
Next review available in: 2 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)
WalkthroughAdds JSX to circular-reference detection and makes JSX props inspection safe when Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude 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 the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 351-353: The test creates el2 with props: 42 and currently only
asserts that an error is thrown when comparing it to {}
(expect(el2).toEqual({})), which allows any thrown error (including ones whose
message incorrectly contains "[Circular]"). Update the assertion for the thrown
error to also assert the error message does NOT contain the string "[Circular]"
so the test fails if a circular-related message is produced; target the existing
el2 setup and the current expect(...).toThrow invocation when making this
change.
🪄 Autofix (Beta)
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: 34ba822f-7961-4803-84d6-bf95dc3fc265
📒 Files selected for processing (2)
src/bun.js/test/pretty_format.zigtest/js/bun/util/inspect.test.js
|
The Supersedes #29170 (same fix, this one has more test coverage). |
There was a problem hiding this comment.
LGTM — the pretty_format.zig mirror fix and the diff-formatter regression test address my earlier comment.
Extended reasoning...
Overview
This PR closes a fuzzer-found native stack overflow in the JSX inspection path. It touches two near-duplicate formatters — src/bun.js/ConsoleObject.zig (used by Bun.inspect/console.log) and src/bun.js/test/pretty_format.zig (used by the test diff renderer) — plus test/js/bun/util/inspect.test.js. In each formatter the change is two lines: add .JSX to canHaveCircularReferences() so the existing visited-map guard fires, and replace props.getObject().? with orelse break :props so a non-object props is treated as no-props instead of panicking. Five new tests cover circular props/children/key, non-object props, and the expect().toEqual() diff-formatter path.
Security risks
None. This is purely output-formatting / crash-avoidance code; it does not touch auth, filesystem, network, or parsing. Both edits are strictly defensive — they only add a cycle guard for one more tag and remove a force-unwrap.
Level of scrutiny
Low-to-medium. The change follows the exact pattern already used for .Array/.Object/.Map/.Set in the same function, so there's no novel logic. The labeled break :props exits after the defer this.quote_strings = prev_quote_strings is registered, so state is restored correctly. The only behavioural surface beyond crash-fix is that JSX values now occupy a slot in the visited map during recursion, which is the intended effect.
Other factors
My previous review flagged that pretty_format.zig was unpatched; the author applied the same fix in f26c1c3 and added a test that drives JestPrettyFormat via expect(el).toEqual({}). No CODEOWNERS cover these files. A bot flagged a possible duplicate (#29170), which maintainers may want to close, but that doesn't affect this PR's correctness.
f26c1c3 to
39e75db
Compare
There was a problem hiding this comment.
LGTM — the mirrored fix in pretty_format.zig addresses my earlier comment, and both code paths now have test coverage.
Extended reasoning...
Overview
This PR fixes a fuzzer-found native stack overflow when Bun.inspect() (and the test-runner diff formatter) encounters a self-referential JSX element. The change touches two near-identical formatters — src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig — adding .JSX to canHaveCircularReferences() so the existing visited-map / [Circular] guard applies, and replacing a props.getObject().? force-unwrap with orelse break :props so a non-object props is skipped instead of asserting. Five new tests in inspect.test.js cover circular props/children/key, non-object props, and the expect().toEqual() diff path.
Security risks
None. This is purely output-formatting / crash-hardening code with no auth, crypto, filesystem, or network surface. The change strictly narrows behavior from a segfault to printing [Circular] or skipping invalid props.
Level of scrutiny
Low-to-medium. The fix is two mechanical edits per file: one enum case added to an existing switch, and one optional-unwrap converted to a labeled-block bailout. The defer this.quote_strings = prev_quote_strings placed before the break :props still fires on early exit, so state is correctly restored. The same pattern (visited-map guarded by canHaveCircularReferences()) already protects .Array/.Object/.Map/.Set, so adding .JSX follows the established design exactly.
Other factors
I previously flagged that the original revision only patched ConsoleObject.zig; the author applied the identical fix to pretty_format.zig in f26c1c3 and added a regression test exercising the diff-formatter path (expect(circularJsx).toEqual({})). That comment is resolved. CodeRabbit's remaining suggestion to tighten the props: 42 diff-formatter assertion is a minor test-quality nit — the test as written already regresses the crash (prior behavior was a segfault, not a thrown error). The CI failures listed by robobun are unrelated flakes (vendor-tarball 502 on darwin-x64, plus known-flaky tests in heap-snapshot/bake/webview/v8-date-parser that this PR does not touch). The bug-hunting system found no issues on the current revision.
There was a problem hiding this comment.
LGTM — the mirrored fix in pretty_format.zig addresses my earlier comment; the remaining .Event note is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes a fuzzer-found native stack overflow in Bun.inspect() for self-referential JSX elements. It makes two small changes, mirrored across src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig: (1) add .JSX to canHaveCircularReferences() so the visited-map / [Circular] guard applies, and (2) replace the props.getObject().? force-unwrap with orelse break :props so a non-object props no longer panics. Five new tests in test/js/bun/util/inspect.test.js cover circular props, children, key, non-object props, and the test-diff-formatter path.
Security risks
None. This is purely a crash-hardening change in the value-formatter; it does not touch auth, network, filesystem, or parsing of untrusted input beyond making an existing code path more defensive.
Level of scrutiny
Low. Each Zig change is a one-line edit on a well-understood guard, applied identically in two near-duplicate formatters. The labelled-block break :props is the idiomatic Zig pattern already used elsewhere in these files. The change can only reduce crashes — it cannot regress correct output for well-formed JSX, since real React elements always have an object props and the visited-map check is a no-op on the first visit.
Other factors
My earlier review asked for the fix to be mirrored into pretty_format.zig; the author did so and added a regression test that exercises the diff-formatter path (expect(el).toEqual({})). The one new inline finding (.Event still omitted from pretty_format.zig's circular-reference list) is explicitly flagged as pre-existing, requires Object.defineProperty to trigger, and is a one-token follow-up the author can take or leave — it does not block this fuzzer fix. CodeRabbit's test-tightening suggestion is a minor nit. CI failure was an unrelated GitHub 502 on vendor tarball fetch.
There was a problem hiding this comment.
LGTM — targeted crash fix with good test coverage; the .Event note is pre-existing and can be a follow-up.
Extended reasoning...
Overview
This PR fixes a fuzzer-found native stack overflow in Bun.inspect() when formatting self-referential JSX elements. The change is ~6 lines of logic across two near-duplicate formatters (src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig): adding .JSX to canHaveCircularReferences() so the existing visited-map guard fires, and replacing a props.getObject().? force-unwrap with orelse break :props so non-object props is skipped instead of asserting. Five new test cases in inspect.test.js cover circular props/children/key, non-object props, and the test-diff-formatter path.
Security risks
None. This is output-formatting code with no auth, crypto, filesystem, or network surface. The change strictly narrows behavior (turning two crash paths into bounded [Circular] / no-op output) and introduces no new inputs or side effects.
Level of scrutiny
Low-to-moderate. The fix is mechanical — it extends an existing enum allowlist by one variant and replaces a panic-on-null with an early break, both well-established patterns already used for other tags in the same functions. The labelled-block break :props correctly preserves the defer this.quote_strings = prev_quote_strings restore. Both copies of the formatter received identical edits.
Other factors
My earlier review flagged that pretty_format.zig needed the same fix; the author applied it and the thread is resolved. The remaining open comments are (1) CodeRabbit's minor test-tightening suggestion and (2) my own note that .Event is still missing from pretty_format.zig's list — both are non-blocking nits about pre-existing behavior, not regressions introduced here. Test coverage directly exercises the previously-crashing inputs in both formatter code paths.
There was a problem hiding this comment.
LGTM — thanks for addressing the test nits; the remaining .Event note is pre-existing and can be a follow-up.
Extended reasoning...
Overview
This PR fixes a fuzzer-found native stack overflow in Bun.inspect() / the test diff formatter when given self-referential JSX-shaped objects. The change is two-fold across both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig: (1) add .JSX to canHaveCircularReferences() so the existing visited-map guard applies, and (2) replace the props.getObject().? force-unwrap with orelse break :props so non-object props is treated as no-props instead of asserting. Three subprocess regression tests are added to test/js/bun/util/inspect.test.js.
Security risks
None. This is a defensive fix in output-formatting code — it strictly reduces crash surface by adding cycle detection and removing an unsafe unwrap. No new inputs, no auth/crypto/permission logic.
Level of scrutiny
Low. The Zig changes are mechanical: one enum tag appended to an existing comptime allowlist (identical pattern already used for .Array/.Object/etc.), and one .? swapped for orelse break :props on a labeled block. Both edits are mirrored verbatim across the two duplicate formatters. The visited-map machinery being enabled is pre-existing and well-exercised by other tags.
Other factors
- My earlier feedback (mirror the fix into
pretty_format.zig; test-convention nits around assertion order,tempDir, and the negativepaniccheck) has all been addressed in commits 39e75db and 8f62f57. - The one open note I left —
.Eventstill missing frompretty_format.zig'scanHaveCircularReferences()— is a pre-existing gap with a contrived trigger and is reasonable to defer to a follow-up; it does not block this fix. - The reported CI failures on 56394e7 are build-infra (
scripts/build/ci.ts, vendor tarball fetches) unrelated to the Zig/test changes here, consistent with the two "Retrigger CI" commits. - Test coverage is solid: circular via
props,children(single + array), andkey; non-objectprops; and theexpect().toEqual()diff path — all run in subprocesses so a regression segfault fails the test rather than killing the runner.
There was a problem hiding this comment.
LGTM — thanks for addressing the test-convention nits. The remaining .Event note on pretty_format.zig:329 is pre-existing and fine as a follow-up.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, and replaces the props.getObject().? force-unwrap with orelse break :props in both files. Three subprocess regression tests are added to test/js/bun/util/inspect.test.js covering circular props/children/key, non-object props, and the test-diff-formatter path.
Security risks
None. This is a defensive change in display/formatting code — it strictly removes a native stack overflow and an assertion failure on malformed JSX-shaped objects. No new inputs are trusted, no parsing/auth/crypto is touched.
Level of scrutiny
Low-to-medium. The fix is a one-token enum addition plus replacing a panic-on-null with a graceful early-out, mirrored across two near-identical formatters. The pattern matches how the other recursive tags (.Array, .Object, .Map, .Set) already participate in the visited-map guard, and ConsoleObject.zig already listed several other tags here. The labelled-block break :props correctly preserves the defer this.quote_strings = prev_quote_strings cleanup.
Other factors
- All earlier review feedback has been addressed: the duplicate formatter in
pretty_format.zigwas patched (f26c1c3), and the test-convention nits (assertion order,tempDir, dropping the negativepaniccheck) were fixed in 8f62f57. - The one open inline comment (
.Eventmissing frompretty_format.zig:329) is my own and explicitly marked as a pre-existing issue of the same class — it does not block this fuzzer-found crash fix. - Tests run the crashing inputs in subprocesses so a regression would fail the suite rather than segfault the runner.
- Reported CI failures on earlier commits were infra-level build-zig/build-cpp flakes unrelated to these Zig/JS-only changes; the author has retriggered.
There was a problem hiding this comment.
LGTM — minimal, well-tested crash fix; all prior review feedback addressed. The remaining .Event note is pre-existing and fine to defer.
Extended reasoning...
Overview
This PR fixes a fuzzer-found native stack overflow in Bun.inspect() (and the Jest diff formatter) when a JSX-shaped object references itself. The Zig changes are tiny and surgical: (1) add .JSX to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig so the existing visited-map / [Circular] guard fires, and (2) replace props.getObject().? with orelse break :props so a non-object props no longer panics. Three new subprocess-based tests in test/js/bun/util/inspect.test.js cover circular props/children/key, non-object props, and the test-runner diff path.
Security risks
None. This is read-only formatting/inspection code; the change strictly narrows crash surface by adding cycle detection and removing a force-unwrap. No auth, crypto, filesystem, or network paths touched.
Level of scrutiny
Low–medium. The logic change is ~4 lines per file and follows the exact pattern already used for .Array/.Object/etc. The orelse break :props is strictly safer than the prior .? unwrap. Tests run in subprocesses so a regression segfault would still produce a clean failure rather than killing the suite.
Other factors
All earlier review feedback has been addressed: the fix was mirrored into pretty_format.zig (f26c1c3), and the test-convention nits — assertion order, tempDir over tmpdirSync, dropping the negative panic check — were applied in 8f62f57. My outstanding 🟣 note about .Event in pretty_format.zig is a pre-existing, contrived-trigger issue I explicitly flagged as optional; not a blocker for this PR. The single CI failure (test-http-should-emit-close-when-connection-is-aborted.ts timeout on Windows) is unrelated to these changes.
89b8a68 to
b36219f
Compare
195aec0 to
8150329
Compare
There was a problem hiding this comment.
LGTM — thanks for addressing the test-convention nits. The pre-existing .Event gap in pretty_format.rs can be a follow-up.
Extended reasoning...
Overview
Two one-line additions: Tag::JSX is added to the can_have_circular_references() allowlist in both src/jsc/ConsoleObject.rs (Bun.inspect / console.log) and src/runtime/test_runner/pretty_format.rs (expect().toEqual() diff renderer). This enables the existing visited-map / [Circular] guard for JSX elements, fixing a fuzzer-found native stack overflow when a React element references itself. ~80 lines of subprocess-based regression tests cover circular props, children, key, and non-object props in both formatters.
Security risks
None. This is a defensive hardening change in output-formatting code — it gates more inputs through an existing cycle guard rather than adding new logic. No auth, crypto, parsing, or untrusted-input boundaries are touched beyond making inspection of pathological objects safer (turning a segfault into [Circular] text).
Level of scrutiny
Low-to-moderate. The Rust changes are mechanically trivial — adding one enum variant to two matches!() arms to opt into pre-existing safety machinery. There is no new control flow; the visited-map logic itself is unchanged. The test additions follow repo conventions (subprocess isolation so a regression fails the suite rather than segfaulting the runner, tempDir with using, stdout asserted before exitCode).
Other factors
All prior review feedback has been addressed: the fix was mirrored into pretty_format.rs after my first-round comment, and the latest revision applies all three test-convention nits from my second-round comment (assertion order, removed not.toContain("panic"), tempDir instead of tmpdirSync). The one remaining note — .Event still missing from pretty_format.rs's allowlist — is a pre-existing issue with a contrived trigger that I explicitly marked as optional; it should not block this fuzzer-fix PR. CodeRabbit's test-tightening suggestion is a minor nice-to-have that does not affect correctness. No bugs were found by the bug-hunting system on the current revision.
8150329 to
68babdc
Compare
There was a problem hiding this comment.
LGTM — the test-convention nits are addressed and the two-line native fix is straightforward; the pre-existing .Event gap I flagged earlier can land separately.
Extended reasoning...
Overview
This PR adds Tag::JSX to can_have_circular_references() in two places — src/jsc/ConsoleObject.rs (the Bun.inspect/console.log formatter) and src/runtime/test_runner/pretty_format.rs (the expect().toEqual() diff formatter) — so that the visited-map cycle guard applies when recursing into JSX props/key/children. Without it, a self-referential React element overflowed the native stack and segfaulted. Three subprocess-isolated regression tests are added to test/js/bun/util/inspect.test.js covering circular props, circular children (single and array), circular key, non-object props, and the test-runner diff path. I verified that pretty_format.rs:2191 already handles non-object props via let Some(props_obj) = props.get_object() else { return Ok(false); }, so the second half of my original 2026-04-25 feedback is covered even though it doesn't appear in this diff.
Security risks
None. This is output-formatting code with no auth, network, filesystem, or crypto surface. The change strictly narrows behavior (adds a cycle guard); it cannot expose data or bypass anything.
Level of scrutiny
Low. The native change is a one-token addition to a matches!() arm in each of two files, following the exact pattern already used for Array/Object/Map/Set (and Function/Error/Class/Event in ConsoleObject). The mechanism — insert into the visited map before recursing, print [Circular] on hit — is unchanged; only the set of tags it applies to grows by one.
Other factors
All feedback from my earlier reviews has been addressed: the pretty_format mirror fix is in, assertion order now puts stdout before exitCode, the not.toContain("panic") line is gone, and the diff-formatter test uses using dir = tempDir(...). The one open item is my 🟣 note that Tag::Event is still missing from pretty_format.rs's list — I confirmed at line 509 it's still Array | Object | Map | Set | JSX — but I explicitly flagged that as pre-existing and non-blocking, and it's a contrived trigger (Object.defineProperty on a DOM event) unrelated to the fuzzer finding this PR fixes. CodeRabbit's unresolved nit about tightening the toThrow() assertion is covered in practice by the outer "2 pass" + exitCode === 0 checks. No new bugs found in this run.
robobun
changed the title
|
Superseded by #34889, which includes this change and also moves the stack check ahead of the |
| pub const fn can_have_circular_references(self) -> bool { | ||
| matches!(self, Tag::Array | Tag::Object | Tag::Map | Tag::Set) | ||
| matches!( | ||
| self, | ||
| Tag::Array | Tag::Object | Tag::Map | Tag::Set | Tag::JSX | Tag::Event | ||
| ) |
There was a problem hiding this comment.
🟣 The stack_check.is_safe_to_recurse() reorder from ConsoleObject.rs has no analogue here — pretty_format.rs has no stack_check at all, so a deep non-cyclic chain (JSX or plain Object/Array) routed through expect(deep).toEqual({}) / snapshot diffs still overflows the native stack and segfaults bun test. This is pre-existing (the file has never had a native-stack guard for any tag) and adding one means introducing a StackCheck field the file does not have today, so likely follow-up scope — just noting that the description's "Deep non-cyclic JSX now throws a catchable RangeError" only holds for the Bun.inspect / console.log path, not the diff formatter.
Extended reasoning...
What the gap is
This PR applies two independent recursion guards in src/jsc/ConsoleObject.rs:
- Adding
Tag::JSXtocan_have_circular_references()so the visited-map /[Circular]guard fires on cyclic JSX. - Moving
self.stack_check.is_safe_to_recurse()ahead of theif !can_circ { return Ok(true) }early return inprint_as_prelude, so every recursiveprint_ascall — regardless of tag — checks the native stack and throws aRangeErrorbefore it overflows.
Fix (1) is mirrored into src/runtime/test_runner/pretty_format.rs (this hunk). Fix (2) is not — and cannot be trivially mirrored — because pretty_format.rs has no stack_check field, no is_safe_to_recurse() call, and no StackCheck import anywhere in the file (grep confirms zero hits). Its print_as prelude's only recursion defence is the visited map, gated on FORMAT.can_have_circular_references().
Why the visited map is not sufficient
The visited map catches cycles (the same JSValue appearing twice on the current recursion stack). It does nothing for a deep chain of distinct objects, because every insert is a fresh key that never hits. A ~20k-deep non-cyclic JSX children chain — one of the three crashing repros the PR description enumerates — is exactly this shape: 20k distinct react-element objects, each the sole child of the previous one. The Tag::JSX arm of JestPrettyFormat::print_as recursively calls self.format(...) on props.children, so this chain drives ~20k native stack frames with nothing to stop it.
Step-by-step proof
- Build a deep non-cyclic JSX tree in a test file:
let el = { $$typeof: Symbol.for("react.element"), type: "div", props: {}, key: null }; for (let i = 0; i < 20000; i++) el = { $$typeof: Symbol.for("react.element"), type: "div", props: { children: el }, key: null }; expect(el).toEqual({});
toEqualfails anddiff_format.rscallsJestPrettyFormat::format(received, …)to renderel.Tag::get(el)→Tag::JSX;print_asis entered.FORMAT.can_have_circular_references()is nowtrue(this PR addedTag::JSX), so the visited map is consulted — butelhas never been seen (each of the 20k elements is a distinct object), so it inserts and continues.- The
Tag::JSXarm readsprops.children, finds the next distinct element, and callsself.format(...)on it → back to step 3 with a fresh value. - There is no
stack_check.is_safe_to_recurse()anywhere on this path, and no JS frame for JSC's recursion limiter to see. After ~20k frames the native stack is exhausted → SIGSEGV inbun test.
The same holds for a 20k-deep {a:{a:{a:...}}} plain-object chain — Tag::Object was already in can_have_circular_references() before this PR, and the visited map likewise never hits on distinct objects.
Why this is pre-existing, not a defect this PR introduced
pretty_format.rs has never had a native-stack recursion guard, for any tag. Deep non-cyclic Object / Array / Map / Set chains through the diff formatter could segfault bun test before this PR and still can after it; Tag::JSX is just one more tag with the same long-standing exposure. The ConsoleObject.rs fix was a reorder of an existing stack_check field; mirroring it here means adding a StackCheck field to JestPrettyFormat, initializing it, and gating print_as on it — net-new infrastructure rather than the one-token additions this PR made to the file. The PR is a strict improvement (cyclic JSX through the diff formatter no longer crashes) and does not make the non-cyclic case worse.
Impact
A user who passes a very deep non-cyclic structure to a failing expect().toEqual() / toMatchSnapshot() will still see bun test die with SIGSEGV rather than a catchable RangeError. The trigger — a ~20k-deep tree of distinct objects handed to a failing matcher — is contrived compared to the cyclic-JSX case a real React DOM produces (which this PR does fix). The PR description's statement "Deep non-cyclic JSX now throws a catchable RangeError instead of segfaulting" is accurate for Bun.inspect / console.log (all three repros in the description are bun -e '…console.log(e)'), but does not hold for the JestPrettyFormat path.
Suggested follow-up
Add a stack_check: bun_core::StackCheck field to JestPrettyFormat and, at the top of print_as (before the visited-map block), do the same if !self.stack_check.is_safe_to_recurse() { …throw RangeError… } gate ConsoleObject.rs:3391 now runs unconditionally. That closes the deep-non-cyclic case for every tag in the diff formatter in one place. Reasonable to land as a follow-up PR given it is net-new infrastructure in a file this PR only touched for the visited-map fix.
There was a problem hiding this comment.
Agreed this is follow-up scope: pretty_format.rs has no StackCheck field today, so adding one is net-new infrastructure rather than the one-line enum additions this PR makes. The Bun.inspect/console.log path (which all three PR-description repros use) is covered. Leaving the diff-formatter stack guard for a separate change.
There was a problem hiding this comment.
LGTM — all prior feedback addressed.
What was reviewed:
print_as_preludereorder: stack check now runs for every tag;Ok(false)path setsself.failedand callers bail cleanly.print_proxyloop: Proxy chains cannot be cyclic (target is fixed at construction), so the unbounded loop terminates;Tag::geterrors propagate via?.pretty_format.rsEvent fallback: verified themap.remove(&value)before Object re-dispatch is correct and the double-remove at line 2603 afterward is a harmless no-op; MessageEvent/ErrorEvent arms recurse on.data/.error(different value) so no remove needed there.- Checked neither JSX printer re-dispatches on the same
value, so addingTag::JSXtocan_have_circular_referencescannot produce spurious[Circular].
Extended reasoning...
Overview
Three files: src/jsc/ConsoleObject.rs (add Tag::JSX to can_have_circular_references, move stack_check.is_safe_to_recurse() ahead of the !can_circ early return, convert print_proxy recursion to a peel loop), src/runtime/test_runner/pretty_format.rs (add Tag::JSX and Tag::Event to can_have_circular_references, add map.remove(&value) before the Event→Object re-dispatch), and six new subprocess tests in test/js/bun/util/inspect.test.js. Net +186/-13.
Security risks
None. This is output-formatting code with no auth/crypto/permission surface. The change strictly hardens against native stack overflow on adversarial input to Bun.inspect / console.log — replacing SIGSEGV with either [Circular], a catchable RangeError, or successful output.
Level of scrutiny
Moderate. Crash fix in a hot-ish diagnostic path, but the mechanism is straightforward: (1) one enum arm added to a const matches!(), (2) two adjacent guard blocks swapped so the cheaper stack check runs unconditionally, (3) tail recursion turned into a loop. The one non-obvious hazard — spurious [Circular] when a newly-guarded tag re-dispatches on the same value — was caught in the previous review round for Tag::Event and is now fixed with a regression test; I re-checked both JSX printers and neither re-dispatches on value itself.
Other factors
This is my fourth pass. All prior findings (test assertion order, tempDir vs tmpdirSync, empty-stderr assertion, missing deep-non-cyclic-JSX test, missing Tag::Event, spurious [Circular] on CloseEvent/CustomEvent) have been addressed in the current diff. The gate evidence in the PR body shows fails-on-main / passes-on-PR under both ASAN debug and release. The one remaining note (no stack_check in pretty_format.rs) is pre-existing across all tags, not a regression, and the author has correctly scoped it to a follow-up. The bug-hunting system found nothing new this run.
50808a2 to
8a36f99
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Re-dispatch as Object on the same value: remove it from the | ||
| // visited map first so the inner call doesn't see a spurious cycle. |
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.
The Event re-dispatch fix from my last review (ed29686) looks correct — map.remove(&value) is in place before the print_as::<Object> fallback, and the new "Event in test diff formatter" test covers CloseEvent/CustomEvent (properties render) plus circular MessageEvent (still [Circular]). All prior review feedback is addressed. Deferring the merge call to a human given the stated overlap with #34889 ("one of the two should absorb the other's remaining pieces").
Reviewed: the Tag::JSX addition doesn't introduce a spurious-[Circular] re-dispatch in print_jsx (it only recurses into child values, never value itself); the print_proxy loop can't cycle since Proxy targets are fixed at construction; and the stack-check reorder now guards every tag on entry.
Extended reasoning...
Overview
This PR fixes three native-stack-overflow SIGSEGVs in Bun.inspect / console.log: cyclic JSX props.children, deep non-cyclic JSX chains, and deep Proxy chains. Changes span src/jsc/ConsoleObject.rs (add Tag::JSX to can_have_circular_references(), move stack_check.is_safe_to_recurse() ahead of the !can_circ early return, convert print_proxy from recursion to a peel loop), src/runtime/test_runner/pretty_format.rs (add Tag::JSX | Tag::Event to the circular set, plus map.remove(&value) before the Event→Object re-dispatch), and six new subprocess tests in test/js/bun/util/inspect.test.js.
Prior review resolution
I've reviewed this PR three times previously. All substantive concerns are now addressed: test-convention nits (assertion order, tempDir, no-panic checks) were fixed; the missing deep-non-cyclic-JSX test and the stderr === "" flake vector were fixed in e0d1df6; and my 🔴 about Tag::Event causing spurious [Circular] on CloseEvent/CustomEvent in the diff formatter was fixed in ed29686 with a regression test. I verified print_jsx in ConsoleObject.rs never re-dispatches print_as on the same value (only on key_value/property_value/children), so adding Tag::JSX to the circular set there does not create the analogous hazard. The print_proxy loop terminates because Proxy [[ProxyTarget]] is set once at construction and only ever transitions to null on revoke — no cycle is constructible.
Security risks
None. This is output formatting; no parsing of untrusted data into privileged operations, no auth/crypto/permission surfaces.
Level of scrutiny
Medium-high. ConsoleObject.rs is the formatter behind every console.log and Bun.inspect call, so a regression is highly visible. That said, the diff is small and mechanical (one enum arm, a two-block reorder, recursion→loop), the mechanism is clearly explained, and the evidence block shows both ASAN-debug and release lanes passing with the new tests failing on main.
Other factors
Two open items keep this from an auto-approve: (1) the PR description explicitly flags overlap with #34889 and says one PR should absorb the other — that merge-ordering decision needs a maintainer; (2) a comment-cop bot flag landed today on the two-line comment at pretty_format.rs:1876-1877. The comment is short and explains a non-obvious map.remove (mirroring the documented remove_before_recurse pattern in ConsoleObject.rs), so I don't think it warrants a code change, but it's a fresh unresolved item. My earlier 🟣 note about pretty_format.rs lacking a native StackCheck remains acknowledged as follow-up scope and is not blocking.
…SEGV) Tag::JSX was missing from can_have_circular_references() in both the console formatter and the test diff formatter, so a React element that referenced itself through props, key, or children recursed until the native stack overflowed. Also run the stack-depth check for every tag (not only those tracked in the visited map), unwrap nested Proxy targets iteratively, add Tag::Event to the diff formatter's visited set, and remove the value before the Event->Object re-dispatch so CloseEvent/CustomEvent do not render as [Circular].
8a36f99 to
68e4811
Compare
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed and the latest commit (02da7db) drains both pipes in every new subprocess test.
What was reviewed:
print_as_preludereorder: stack check now runs unconditionally before the!can_circearly return;is_safe_to_recurse()is side-effect-free so this is safe for all tags.print_proxyloop: Proxy target chains cannot cycle (each target must exist before the Proxy wrapping it), so the peel loop terminates; revoked proxies get a distinct tag and exit viaformat.pretty_format.rsEvent re-dispatch:map.remove(&value)before theTag::Objectre-entry mirrors ConsoleObject'sremove_before_recursepattern; the new CloseEvent/CustomEvent/circular-MessageEvent test locks it in.- Six subprocess tests: async
Bun.spawn+it.concurrent, both stdout and stderr drained inPromise.all,signalCodeasserted falsy on the deep-chain crashers.
Extended reasoning...
Overview
This PR fixes three native-stack-overflow SIGSEGVs in Bun.inspect / console.log: cyclic JSX props.children, deep non-cyclic JSX chains, and deep Proxy target chains. Changes touch src/jsc/ConsoleObject.rs (three small hunks: add Tag::JSX to can_have_circular_references, move the stack-safety check ahead of the !can_circ early return in print_as_prelude, and convert print_proxy's target recursion to a loop), src/runtime/test_runner/pretty_format.rs (add Tag::JSX | Tag::Event to the circular set and map.remove(&value) before the Event→Object re-dispatch), and six new subprocess crash-guard tests in test/js/bun/util/inspect.test.js.
Security risks
None. This is the console/inspect formatter — it renders values, does not parse untrusted input into privileged operations, and the fix strictly reduces attack surface (a hostile object graph could previously SIGSEGV the process; now it prints [Circular] or throws a catchable RangeError). No auth, crypto, filesystem, or network paths touched.
Level of scrutiny
Medium. The formatter is user-visible but not correctness-critical to program semantics — it only affects diagnostic output. The changes are minimal and mechanical: an enum-match arm addition, a two-block reorder of side-effect-free checks, and a recursion→loop conversion. I traced each:
- The stack-check reorder is safe because
is_safe_to_recurse()only reads the stack pointer; running it for non-circular tags adds no behavior beyond the intended guard. - The Proxy peel loop cannot infinite-loop: Proxy target chains are acyclic by construction (each target must exist before the Proxy is created), and a revoked nested proxy would return a non-
Tag::ProxyfromTag::getand exit viaformat. - The
map.remove(&value)before re-dispatch inpretty_format.rsmirrors the exactremove_before_recursepattern already documented inConsoleObject.rs:3335for the same hazard.
Other factors
This PR has been through five review rounds; every prior finding (missing deep-non-cyclic-JSX test, exact-empty-stderr assertion, spurious-[Circular] on Event re-dispatch, spawnSync vs async, undrained pipes) was addressed with a follow-up commit and a regression test where applicable. The one open informational note (pretty_format.rs lacks a StackCheck field entirely, so deep non-cyclic chains through the diff formatter can still overflow) is pre-existing, acknowledged by the author as follow-up scope, and not introduced by this PR. The description flags overlap with #34889 for maintainer coordination, but the code here is independently correct and mergeable.
1 participant
console.log/Bun.inspectsegfaults (native stack overflow, exit 139, no crash report) on:A ~20k-deep non-cyclic JSX
childrenchain crashes the same way. Node prints all three.Cause
print_as_preludeonly ranstack_check.is_safe_to_recurse()and the visited-map insert for tags incan_have_circular_references().Tag::JSXwas not in that set, so the JSX printer recursed intoprops/childrenwith neither guard.Tag::Proxywas also not in the set, andprint_proxyrecursed throughformat()on the target, so a deep Proxy chain blew the native stack before ever reaching a guarded tag. There is no JS frame on this path, so JSC's recursion limiter never fires.Fix
Tag::JSXtocan_have_circular_references()(in bothConsoleObject.rsand the test-runnerpretty_format.rs) so cyclic JSX prints[Circular].stack_check.is_safe_to_recurse()check ahead of thecan_circearly return inprint_as_prelude, so every recursiveprint_ascall is guarded regardless of tag. Deep non-cyclic JSX now throws a catchableRangeErrorinstead of segfaulting.print_proxypeel nested Proxy targets in a loop instead of recursing, so a 100k-deep Proxy chain prints the innermost target without consuming stack.Verification
New tests in
test/js/bun/util/inspect.test.jsrun the crashing inputs in subprocesses so a regression fails the suite rather than killing the runner.Related: #10886. That issue's scenario (a happy-dom element printed by a jest-dom matcher) is a different bug: the element is a plain object, so it already goes through the circular set and depth limit and prints as a bounded but very long object dump. It is not fixed here; #35712 (DOM nodes printed as markup) is the fix for it. This PR is the SIGSEGV class only, so it does not close #10886.
Overlaps with #34889, which carries the
Tag::JSXcircular entry and the stack-check reordering from here plus a liveStackCheckinFormatter::new()and inpretty_format; the Proxy peeling loop,Tag::Eventinpretty_format's circular set, and the Event re-dispatchmap.removefix (with their tests) exist only in this PR. One of the two should absorb the other's remaining pieces.Supersedes #29170
no test proof · iteration 19 · 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