Fix stack overflow in Bun.inspect for circular JSX elements - #30126
Fix stack overflow in Bun.inspect for circular JSX elements#30126robobun wants to merge 1 commit into
Conversation
|
Updated 7:42 AM PT - May 22nd, 2026
❌ @robobun, your commit d8afc4d has some failures in 🧪 To try this PR locally: bunx bun-pr 30126That installs a local version of the PR into your bun-30126 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesJSX Circular Reference Detection
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, targeted fix with regression tests; the remaining inline note is pre-existing and fine for a follow-up.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both ConsoleObject.zig and pretty_format.zig (one-token change each) plus three regression tests in inspect.test.js. This enables the existing visited-map / stack-safety guard for JSX-tagged values so circular React-element-shaped objects print [Circular] instead of stack-overflowing.
Security risks
None. Pure formatter hardening — strictly reduces crash surface reachable from user JS.
Level of scrutiny
Low. Two one-line additions to an enum predicate, following an established pattern. I verified the .JSX printAs branches in both files only recurse into sub-values (type/key/props/children), never re-entering on the element itself, so there's no spurious-[Circular] hazard like the one that forced the .Event revert in cdfc31b.
Other factors
My two earlier review rounds were addressed: I suggested adding .Event, then flagged the resulting fall-through regression, and the author correctly reverted to .JSX-only and scoped .Event out as a follow-up. The new inline comment about props.getObject().? is explicitly pre-existing and not introduced here. CI noise on build #50297 is from known flakes per the retrigger commit messages. The duplicate-PR bot flagged #29709 — maintainers can dedupe, but that doesn't affect correctness of this change.
There was a problem hiding this comment.
LGTM — minimal and correct fix; the .Event regression was reverted and the remaining inline note is pre-existing.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both ConsoleObject.zig and pretty_format.zig (one token each) so that the visited-map guard in printAs runs for JSX-tagged values, plus three regression tests in inspect.test.js covering circular key, props.children, and arbitrary props.
Security risks
None. This is a formatter-only change that enables an existing cycle-detection guard for one more tag. No new inputs, no auth/crypto/permissions surface.
Level of scrutiny
Low. The fix is the obvious one-line extension of an existing allowlist, and the failure mode it addresses (stack overflow on a fuzzer-crafted circular JSX object) is strictly improved. I verified that the .JSX branch in both files only recurses into key_value / property_value / children — never back into printAs on the same value with a different tag — so there is no spurious-[Circular] regression risk of the kind that affected the briefly-added .Event case.
Other factors
My earlier feedback loop on this PR is fully resolved: the .Event addition that caused a fall-through regression was reverted in cdfc31b, and the remaining inline comment about props.getObject().? is explicitly flagged as pre-existing (not introduced here) and can be handled separately. The new tests directly exercise the three recursion entry points. CI failures on cdfc31b are the known Zig-compiler-ICE / build-cache infra flakes being retriggered in subsequent commits, not related to this diff. A duplicate-PR bot flagged #29709 as covering the same fix; that's a merge-coordination concern for maintainers, not a correctness one.
![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 339-345: The test currently pipes proc.stderr but never consumes
it and asserts exitCode before checking outputs; update the test to
await/collect stderr (e.g., const [stdout, stderr, exitCode] = await
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) or await
separately), filter out ASAN startup warning lines from stderr when bunEnv is
used (use a regex to remove lines like "WARNING: AddressSanitizer:"), assert
stdout and filtered stderr expectations first, and only then assert
expect(exitCode).toBe(0); reference the proc/stdout/stderr/exitCode variables
and bunEnv in your changes.
🪄 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: d1a10d80-8713-4dfa-b693-c713dd808ac6
📒 Files selected for processing (1)
test/js/bun/util/inspect.test.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/bun/util/inspect.test.js (1)
319-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume stderr and assert
exitCodelast.
proc.stderris still piped but never read, and the exit code is checked before any output validation. That can hide failures and can deadlock if the child writes to stderr. Please collect stderr, filter the known ASAN startup warning when usingbunEnv, verify stdout/stderr first, and only then assertexitCode.Suggested fix
- const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); - expect(exitCode).toBe(0); - expect(stdout).toBe("<div key=[Circular] />\n" + "<div>\n [Circular]\n</div>\n" + "<div foo=[Circular] />\n"); + const [stdout, stderr, exitCode] = await Promise.all([ + proc.stdout.text(), + proc.stderr.text(), + proc.exited, + ]); + const stderrLines = stderr + .split("\n") + .filter(line => line.length > 0) + .filter(line => !line.startsWith("WARNING: ASAN interferes")); + expect(stderrLines).toEqual([]); + expect(stdout).toBe("<div key=[Circular] />\n" + "<div>\n [Circular]\n</div>\n" + "<div foo=[Circular] />\n"); + expect(exitCode).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/bun/util/inspect.test.js` around lines 319 - 344, The test spawns proc with stderr piped but never reads it and asserts exitCode before verifying outputs; modify the test to concurrently read proc.stdout.text() and proc.stderr.text(), filter known ASAN startup warning from stderr when bunEnv is used, then assert stdout and (filtered) stderr contents first, and only after validating outputs assert exitCode; refer to symbols proc, stdout, stderr, exitCode, Bun.inspect and bunEnv to locate and update the spawn result handling and expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 319-344: The test spawns proc with stderr piped but never reads it
and asserts exitCode before verifying outputs; modify the test to concurrently
read proc.stdout.text() and proc.stderr.text(), filter known ASAN startup
warning from stderr when bunEnv is used, then assert stdout and (filtered)
stderr contents first, and only after validating outputs assert exitCode; refer
to symbols proc, stdout, stderr, exitCode, Bun.inspect and bunEnv to locate and
update the spawn result handling and expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f69d7049-4c9f-4256-9b5a-75b52829676c
📒 Files selected for processing (1)
test/js/bun/util/inspect.test.js
There was a problem hiding this comment.
LGTM — adding .JSX to both canHaveCircularReferences() switches is the minimal correct fix, and the test locks in exact output. My remaining inline notes are pre-existing / nit-level and non-blocking.
Extended reasoning...
Overview
Two one-token additions: .JSX is appended to the canHaveCircularReferences() match arms in src/bun.js/ConsoleObject.zig:1139 and src/bun.js/test/pretty_format.zig:329, plus a subprocess regression test in test/js/bun/util/inspect.test.js covering circular key, props.children, and arbitrary-prop cycles. The .Event addition I'd suggested earlier was tried and then correctly reverted (cdfc31b) after I flagged the spurious-[Circular] regression on the fall-through path, so the diff is back to the minimal JSX-only fix.
Security risks
None. This only changes which formatter tags participate in the visited-map cycle guard during value inspection. No auth, crypto, permissions, or untrusted-input parsing is touched; the change strictly adds a safety check.
Level of scrutiny
Low. Adding a tag to an existing enum switch that already handles .Array/.Object/.Map/.Set identically is mechanical. I verified the .JSX printAs branch in both files never re-enters printAs on the same value (unlike .Event's fall-through), so the remove_before_recurse concern doesn't apply here — confirmed by the test's exact-output assertion (<div key=[Circular] />, not bare [Circular]).
Other factors
- The author was responsive across the review cycle: applied the
.Eventsuggestion, then reverted when the regression was identified, scoping this PR back to the JSX crash only. - My two unresolved inline comments are (1) a pre-existing
props.getObject().?unwrap on hand-crafted JSX-like objects — explicitly out of scope and not introduced here — and (2) a nit on test assertion ordering (stderr/stdout before exitCode) that affects only failure diagnostics, not correctness. Neither blocks. - CI failures on the latest build are infra flakes (Zig compiler ICE in
std/process/Child.zig, Azure agent provisioning) per the retrigger commit messages, unrelated to this diff. - A duplicate-PR bot flagged #29709 as covering the same fix; a human can dedupe at merge time, but that doesn't affect correctness of this change.
There was a problem hiding this comment.
LGTM — the test now follows the spawn conventions and the .JSX-only scope is correct.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both src/bun.js/ConsoleObject.zig and src/bun.js/test/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js. This is a one-token addition to two switch/or-chain expressions that gates the existing visited-map / [Circular] machinery, fixing a Fuzzilli-found stack overflow when a hand-crafted React element references itself via key/props/children.
Security risks
None. This only affects the formatter's cycle-detection set; no parsing, auth, or external input handling is touched. The change strictly narrows a crash surface.
Level of scrutiny
Low-to-moderate. The functional diff is two characters of enum membership in well-understood cycle-detection logic. I verified that the .JSX arm of printAs in both files never re-enters printAs on the same value (only on key_value / property_value / children), so there is no spurious-[Circular] hazard of the kind that forced the .Event revert earlier in this PR's history. The existing inspect.test.js JSX cases (jsx with two elements, jsx with fragment, inline-snapshot JSX in the inspect test) cover the non-circular path and would catch any regression there.
Other factors
All earlier review feedback has been resolved: the .Event addition that regressed CloseEvent/CustomEvent snapshots was reverted in cdfc31b; the test was updated in 61823f2 to read stderr, assert it empty, assert stdout, then exitCode — matching CLAUDE.md conventions. The remaining open inline note about props.getObject().? is explicitly pre-existing and out of scope. The three CI failures (s3-storage-class, ssg-pages-router, dev-and-prod) are unrelated to inspect/formatting. github-actions flagged #29709 as a possible duplicate; that's a merge-coordination question, not a correctness concern with this diff.
There was a problem hiding this comment.
LGTM — minimal, targeted fix with a regression test; all earlier review feedback addressed.
Extended reasoning...
Overview
Two one-line Zig changes add .JSX to canHaveCircularReferences() in both src/bun.js/ConsoleObject.zig and src/bun.js/test/pretty_format.zig, so the existing visited-map / [Circular] guard in printAs now applies to JSX elements. A new subprocess test in test/js/bun/util/inspect.test.js exercises three circular shapes (via key, props.children, and a custom prop) and asserts exact output.
Security risks
None. This is output-formatting only; it tightens recursion handling rather than loosening anything. No auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Low. The change is purely additive to an existing enum-tag allowlist that gates cycle detection. I verified that neither formatter's .JSX branch ever re-enters printAs on the same value (it only recurses into key/props/children sub-values), so there is no spurious-[Circular] regression of the kind that the briefly-attempted .Event addition caused — that change was correctly reverted in cdfc31b and is out of scope here.
Other factors
- All four prior inline review threads (mine and CodeRabbit's) are resolved; the test now reads
stderr, asserts it empty, thenstdout, thenexitCode, matching repo conventions. - The remaining open inline comment about
props.getObject().?is explicitly pre-existing and flagged for a follow-up, not a blocker for this PR. - The robobun build-failure banner on de0a61d reflects CI infrastructure flakes (per the "systemic across builds" / "container git state stuck" retrigger commit messages), not a problem with this two-line code change.
- A duplicate-PR bot flagged #29709 as covering the same fix; that's a merge-coordination question for maintainers, not a correctness concern with this diff.
There was a problem hiding this comment.
LGTM — minimal, correct crash fix with a regression test; the ASAN-stderr nit below is non-blocking.
Extended reasoning...
Overview
Two one-token additions: .JSX is added to canHaveCircularReferences() in both src/bun.js/ConsoleObject.zig and src/bun.js/test/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js covering circular references via key, props.children, and an arbitrary prop.
Correctness
The .JSX arm in both formatters recursively calls this.format() on key, prop values, and children, so it clearly needs the visited-map / stack guard. I traced both .JSX printAs branches and confirmed neither re-enters printAs on the same value with a different tag (the pattern that caused the spurious [Circular] when .Event was briefly added and then reverted in cdfc31b). Sibling repeats (e.g. [el, el]) are unaffected because the defer remove runs between iterations. Existing non-circular JSX tests in this file continue to exercise the unchanged happy path.
Security risks
None. This is output formatting only; the change strictly adds a recursion guard to a path that previously had none.
Level of scrutiny
Low. Mechanical extension of an existing enum-set check, mirrors the established pattern for .Array/.Object/.Map/.Set, and is covered by a targeted regression test for a Fuzzilli-found segfault. No CODEOWNERS apply to the touched files.
Other factors
- Earlier review feedback (read/assert stderr, assert exitCode last) was addressed in 61823f2; those threads are resolved.
- The remaining inline nit about filtering the ASAN startup banner from stderr is a possible CI-only flake on the linux-asan job;
test/CLAUDE.md's canonical spawn pattern uses the unfilteredexpect(stderr).toBe(""), so this is non-blocking and CI will surface it if real. - The pre-existing
props.getObject().?crash I flagged earlier is out of scope per the author's reply and predates this PR. - A duplicate-PR bot flagged #29709; that's a process question for maintainers, not a correctness concern here.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); |
There was a problem hiding this comment.
🟡 🟡 Nit: CodeRabbit's suggestion you partially applied in 61823f2 also filtered the WARNING: ASAN interferes with JSC signal handlers… banner that ASAN builds write to stderr at startup (see expectations.txt:15 and the 17 sibling subprocess tests — e.g. string_decoder.test.js:303-307, setTimeout.test.js:429 — that strip it before asserting empty stderr). With a raw expect(stderr).toBe("") this test may go red on the linux-asan job; the one-line filter is cheap insurance:
expect(stderr.split("\n").filter(l => l && !l.startsWith("WARNING: ASAN interferes"))).toEqual([]);Extended reasoning...
What this is
Commit 61823f2 addressed the earlier review by reading proc.stderr.text() and reordering the assertions, but dropped one piece of CodeRabbit's committable suggestion (inline comment 3177287793): the filter for the ASAN startup banner. On linux ASAN builds, the bun-asan binary may write WARNING: ASAN interferes with JSC signal handlers; … to stderr during JSC initialization (the comment at string_decoder.test.js:303 attributes it to WebKit's Options.cpp, which is not in this checkout). bunEnv (test/harness.ts:72-74) sets ASAN_OPTIONS=allow_user_segv_handler=1:disable_coredump=0, which does not suppress that banner. With the new expect(stderr).toBe("") at line 342, the linux-asan job would see received: "WARNING: ASAN interferes with JSC signal handlers; …\n" and fail this test.
Why existing safeguards don't prevent it
There is no harness helper that strips this line — every subprocess test that wants to assert empty stderr under ASAN does it inline. Seventeen test files in the repo currently filter exactly this prefix (e.g. test/js/node/string_decoder/string-decoder.test.js:305-308, test/js/web/timers/setTimeout.test.js:429, test/js/bun/resolve/bun-main-entry-point.test.ts:16, test/js/node/buffer-copy-fill-detach.test.ts:23, test/js/bun/udp/udp_socket.test.ts:39, test/regression/issue/29519.test.ts), several with a verbatim comment that "ASAN builds unconditionally print … to stderr at startup". And test/expectations.txt:15 skips child_process.test.ts on ASAN with the note # Unexpected identifier "WARNING" — independent evidence that the banner is real on the ASAN builder, not just a CodeRabbit-learned cargo-cult.
Step-by-step proof
- linux-asan job spawns
bunExe()(=bun-asan) with-e <script>andenv: bunEnv. - During JSC
Options::initialize(), the ASAN-aware path emitsWARNING: ASAN interferes with JSC signal handlers; …\nto stderr before any user code runs. - The script runs, writes the three
<div …>lines to stdout, exits 0. proc.stderr.text()resolves to"WARNING: ASAN interferes with JSC signal handlers; …\n".expect(stderr).toBe("")fails; the stdout/exitCode assertions never run.
Addressing the refutation
One reviewer points out that the literal fprintf is not in src/ (only the comment at ZigGlobalObject.cpp:288), that test/CLAUDE.md's canonical spawn pattern is the unfiltered expect(stderr).toBe(""), and that ~80 other tests use that unfiltered form without being listed as ASAN failures in expectations.txt. That's all accurate, and it's why this is a nit rather than a guaranteed CI break: either the banner is gated on something not every test hits, or the linux-asan job tolerates/curates these. But the expectations.txt skip (Unexpected identifier "WARNING") and the seventeen explicit filters — including one merged the day before this PR in #30058 — are direct evidence that the banner does surface on the ASAN builder for at least some bunExe() spawns. Since CodeRabbit already proposed the filter on this exact diff and the author applied two-thirds of that suggestion, finishing it is the path of least resistance.
Fix
const stderrLines = stderr.split("\n").filter(l => l && !l.startsWith("WARNING: ASAN interferes"));
expect(stderrLines).toEqual([]);(or the two-.filter() form from CodeRabbit's committable suggestion). Test-only, no behavior change on non-ASAN builds.
There was a problem hiding this comment.
LGTM — minimal, correct fix with regression coverage.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both ConsoleObject.zig and pretty_format.zig, plus a subprocess regression test in inspect.test.js covering three circular shapes (key, props.children, arbitrary prop). The earlier .Event addition that regressed the fall-through path was reverted in cdfc31b, and the test was updated in 61823f2 to follow the repo's stderr/stdout/exitCode assertion ordering. Commits since then are CI retriggers only.
Security risks
None. This is purely output-formatting logic; the change strictly adds a cycle guard (visited-map + stack-overflow check) to a code path that previously lacked one. No new untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low. The diff is a one-token addition to two existing switch/boolean expressions. I verified that neither .JSX printAs branch calls this.printAs(..., value, ...) on the same value (only on key/props/children sub-values), so the visited-map insertion cannot produce a spurious [Circular] the way the reverted .Event change did. Existing JSX inspect tests in this file (jsx with two elements, jsx with fragment, the inline <div> cases in the inspect test) cover the non-circular path against regression.
Other factors
All inline review threads are resolved. The two remaining notes from prior runs are explicitly out of scope: the pre-existing props.getObject().? unwrap is unchanged by this PR, and the ASAN stderr-filter suggestion is a self-acknowledged nit (multiple CI retriggers since 61823f2 indicate the test is passing as written). The duplicate-PR bot flagged #29709, but that's a routing concern for maintainers, not a correctness issue with this diff.
a6e7fda to
3659e59
Compare
There was a problem hiding this comment.
LGTM — one-token addition to the existing circular-ref guard in both formatters, with a subprocess regression test; the open ASAN-stderr note is a non-blocking nit.
Extended reasoning...
Overview
The PR adds .JSX to canHaveCircularReferences() in both src/bun.js/ConsoleObject.zig and src/bun.js/test/pretty_format.zig so that the existing visited-map guard in printAs runs for JSX-tagged values, fixing a fuzzer-found stack overflow when a hand-crafted React element references itself via key, props, or children. A subprocess regression test in test/js/bun/util/inspect.test.js exercises all three cycle shapes and asserts [Circular] output with exit code 0.
Security risks
None. This is a defensive change that removes a user-JS-reachable segfault. It does not touch auth, crypto, permissions, or any trust boundary; it only gates an already-existing cycle-detection map on one additional enum variant.
Level of scrutiny
Low. The functional change is literally one token per file, appended to an existing allow-list that already covers .Array, .Object, .Map, .Set, etc. I verified that the .JSX arm of printAs in both formatters never calls this.printAs(..., value, ...) on the same value (it only recurses on type, key, children, and individual prop values), so there is no spurious-[Circular] hazard of the kind that bit the briefly-attempted .Event addition — that experiment was correctly reverted in cdfc31b after review caught the fall-through regression.
Other factors
All earlier inline review threads are resolved: the .Event regression was reverted, the test was updated to read/assert stderr and order exitCode last per repo conventions. The one open thread is my own nit about filtering the ASAN startup banner before expect(stderr).toBe("") — explicitly framed as cheap insurance, not a blocker, and CI will surface it if it actually bites. The pre-existing props.getObject().? unwrap and the duplicate-PR notice (#29709) are both informational and out of scope for this fix's correctness.
ec40846 to
a7a3cfd
Compare
There was a problem hiding this comment.
LGTM — one-token addition to two canHaveCircularReferences() lists with a regression test; CI is green modulo an unrelated http flake, so my earlier ASAN-stderr nit is non-blocking.
Extended reasoning...
Overview
Adds .JSX to Tag.canHaveCircularReferences() in both src/bun.js/ConsoleObject.zig:1139 and src/bun.js/test/pretty_format.zig:329, so the visited-map / [Circular] guard at the top of printAs now fires for JSX-tagged values. A subprocess regression test in test/js/bun/util/inspect.test.js exercises three circular shapes (via key, props.children, and a non-children prop) and asserts the exact output plus clean exit.
Security risks
None. This is a formatter-only change that turns a stack-overflow crash (Fuzzilli fingerprint bb8715595a137556) into a bounded [Circular] print. No new inputs are accepted, no auth/crypto/permissions code is touched, and the test spawns bunExe() with a fixed inline script.
Level of scrutiny
Low. The Zig change is a single enum tag appended to two existing match lists, mirroring the established pattern for .Array/.Object/.Map/.Set. I verified the .JSX arm of printAs in both files never calls this.printAs(..., value, ...) on the same value (it only recurses into key/props/children sub-values), so there is no spurious-[Circular] hazard like the one I flagged when .Event was briefly added — that experiment was reverted in cdfc31b and the diff is back to the minimal JSX-only fix.
Other factors
All earlier review threads are resolved: the .Event regression I flagged was reverted, and the test was updated to read/assert stderr and check exitCode last per repo conventions. My two remaining inline notes are explicitly non-blocking — one is a pre-existing props.getObject().? crash on the same path (out of scope per the author), and the other is an ASAN-banner stderr filter nit that Build #50536 shows did not actually fail on the ASAN job. No CODEOWNERS cover these paths. github-actions flagged a possible duplicate (#29709), which is a process question rather than a correctness one.
There was a problem hiding this comment.
LGTM — one-token addition to an existing cycle guard, verified by the new regression test; remaining inline notes are pre-existing/nit-level.
Extended reasoning...
Overview
Adds .JSX to Tag.canHaveCircularReferences() in both src/bun.js/ConsoleObject.zig and src/bun.js/test/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js. The Zig change is a single enum value appended to an existing switch arm in each file; everything else (the visited-map insert, [Circular] write, and defer remove) is pre-existing machinery that now applies to the JSX tag.
Security risks
None. This is output-formatting code; the change strictly enables an existing safety guard (visited-map cycle detection) for one more tag. No new inputs are parsed, no allocations change, no auth/crypto/permissions surface.
Level of scrutiny
Low. The fix is mechanical: .JSX recursively formats key, props.*, and children via this.format(), so it belongs in the same set as .Array/.Object/.Map/.Set. I verified that the .JSX branch in both formatters never calls printAs(..., value, ...) on the same value (only on child values), so there is no spurious-[Circular] hazard like the one that forced the .Event revert in cdfc31b. The existing JSX tests in inspect.test.js (jsx with two elements, jsx with anon component, jsx with fragment, plus the inline JSX cases in the inspect test) cover the non-circular path and would catch any regression there.
Other factors
- Build #50594 failures are all known systemic flakes (s3-storage-class, hot.test, http-emit-close, dev-and-prod) that this branch has been retriggering for;
inspect.test.jsitself passed on every platform. - The earlier
.Event-in-pretty_format.zigregression I flagged was reverted by the author and is now out of scope; the diff is back to.JSXonly. - My two remaining inline notes are explicitly pre-existing (
props.getObject().?panic on hand-crafted non-object props) and nit (ASAN stderr-banner filter). Neither blocks: the first predates this PR, and the second did not surface as a CI failure on the latest build. - A duplicate PR (#29709) was flagged by the bot; that's a process question for maintainers and doesn't affect this PR's correctness.
7e609a1 to
484a3fd
Compare
There was a problem hiding this comment.
LGTM — the two open inline notes (pre-existing props.getObject().? and the ASAN stderr filter) are both non-blocking nits.
Extended reasoning...
Overview
This PR adds .JSX to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js. The actual source change is two enum-list additions — about as minimal as a fix gets.
Correctness
Unlike .Event (which the author tried and correctly reverted in cdfc31b after I flagged the spurious-[Circular] regression), the .JSX branch in both formatters never re-enters printAs on the same value with a different tag — it only recurses into key, prop values, and children, which are distinct JSValues (or genuinely circular). So adding .JSX to the visited-map gate cannot produce a false [Circular]. Verified against both printAs(.JSX, …) implementations.
Security risks
None. This is output-formatting code; the change strictly tightens behavior (turns a stack overflow into a bounded [Circular] print). No new attack surface.
Level of scrutiny
Low. Fuzzer-found crash fix, two-line source delta, well-understood mechanism (the same visited-map guard already used for .Array/.Object/.Map/.Set), with a regression test that exercises three distinct cycle shapes (via key, props.children, and an arbitrary prop).
Other factors
- All four resolved review threads were addressed correctly (assertion ordering, stderr draining, the
.Eventadd+revert). - Two inline comments remain open but are explicitly non-blocking: the pre-existing
props.getObject().?unwrap (flagged as a follow-up for the same fuzzer family) and a 🟡 nit about filtering the ASAN startup banner from stderr — CI will surface the latter if it's actually load-bearing, and ~80 other tests use the same unfilteredexpect(stderr).toBe(""). - The current ❌ on Build #50983 is a CI-wide agent outage per the retrigger commit message, not a test failure.
- A possible duplicate (#29709) was flagged by the dedup bot; that's a merge-coordination concern, not a correctness one.
There was a problem hiding this comment.
LGTM — one-line additions to canHaveCircularReferences() in both formatters, with a subprocess regression test. The remaining ASAN-stderr filter is a nit; CI will surface it if it matters.
Extended reasoning...
Overview
This PR adds .JSX to Tag.canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, so the visited-map cycle guard at the top of printAs runs for JSX-tagged values. Without it, a hand-crafted React element whose key/props/children referenced itself recursed forever and segfaulted (Fuzzilli fingerprint bb8715595a137556). A subprocess regression test in test/js/bun/util/inspect.test.js exercises all three self-reference shapes and asserts [Circular] in the output.
Security risks
None. This is a crash-hardening fix in the inspect/pretty-print path; it does not touch auth, IO, permissions, or parsing of untrusted input beyond what Bun.inspect already handles. The change strictly adds a guard.
Level of scrutiny
Low. The diff is two one-token edits to existing boolean predicates plus a test. Unlike the briefly-attempted .Event addition (reverted in cdfc31b after I flagged the spurious-[Circular] fall-through), the .JSX branch in both formatters never re-enters printAs on the same value as a different tag — it only recurses into key/props/children, which are distinct JSValues — so there is no remove_before_recurse hazard here.
Other factors
All earlier review threads are resolved: the .Event regression was reverted, and the test now reads/asserts stderr and orders exitCode last per CLAUDE.md. My one open inline comment (filtering the WARNING: ASAN interferes… banner before expect(stderr).toBe("")) is explicitly a nit — test/CLAUDE.md's canonical pattern is the unfiltered form, ~80 sibling tests use it, and the linux-asan job will tell us definitively. The pre-existing props.getObject().? crash I noted is out of scope (author already declined to widen scope for .Event); it predates this PR. The five most recent commits are pure CI retriggers with no code changes since I last reviewed.
There was a problem hiding this comment.
LGTM — the two one-line .JSX additions to canHaveCircularReferences() are correct and the regression test covers all three cycle paths; remaining inline notes are pre-existing/nit-level and non-blocking.
Extended reasoning...
Overview
This PR adds .JSX to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js. The Zig changes are each a single enum value appended to an existing switch/boolean expression — purely additive, gating the visited-map cycle guard that already exists at the top of printAs. I verified neither .JSX arm re-enters printAs on the same value (only on key/props/children sub-values), so there is no spurious-[Circular] risk of the kind that forced the .Event revert in cdfc31b.
Security risks
None. This is output formatting for Bun.inspect/console.log/snapshot serialization. The change strictly adds a guard (visited-map insert + [Circular] short-circuit) to a path that previously had none, converting a user-JS-reachable stack overflow into a safe string. No auth, crypto, permissions, or untrusted-input parsing involved.
Level of scrutiny
Low. The fix is mechanical and mirrors the existing pattern for .Array/.Object/.Map/.Set in the same function. The earlier review cycle already caught and reverted the one risky over-extension (adding .Event to pretty_format.zig without the remove_before_recurse plumbing). What remains is the minimal, obviously-correct version.
Other factors
- The PR has been through several review rounds; the test was reworked per feedback to read/assert stderr and order assertions per repo convention.
- My two open inline comments are explicitly non-blocking: the
props.getObject().?note is pre-existing (not introduced here), and the ASAN stderr filter is a nit I flagged as "not a guaranteed CI break" — ~80 sibling tests use the same unfilteredexpect(stderr).toBe("")without issue. - Current CI failures (build #51406) are infrastructure: a Windows build-cpp agent-creation failure and the known-flaky
test-http-should-emit-close-when-connection-is-aborted.tstimeout — neither related to this diff (recent commits are all "ci: retrigger" for infra outages). - A duplicate PR (#29709) implements the identical fix, corroborating this is the right shape.
There was a problem hiding this comment.
LGTM — minimal, correct fix. The two open inline comments (pre-existing props.getObject().? crash and the ASAN stderr-filter nit) are non-blocking and fine as follow-ups.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig (one token each), plus a subprocess regression test in test/js/bun/util/inspect.test.js covering circular refs via key, props.children, and an arbitrary prop. This enables the existing visited-map / [Circular] guard for JSX elements, fixing a Fuzzilli-found stack overflow.
Security risks
None. Pure formatter/inspect output change; no auth, crypto, parsing, or untrusted-input handling beyond what already exists. The change strictly adds a safety check.
Level of scrutiny
Low. The diff is two enum-predicate additions following the established pattern for .Array/.Object/.Map/.Set. I verified the .JSX arm of printAs in both files never re-enters printAs on the same value (it only recurses into type/key/children/prop values), so there is no spurious-[Circular] regression of the kind that adding .Event caused (and which was correctly reverted in cdfc31b).
Other factors
All earlier blocking review feedback was addressed: the .Event over-reach was reverted, and the test was updated to drain stderr and assert stdout/stderr before the exit code per repo conventions. The two unresolved inline comments are my own and explicitly non-blocking — one is a pre-existing crash on the same code path (props.getObject().? with non-object props), the other is an optional ASAN stderr-banner filter; neither is introduced by this PR. Recent commits are CI infrastructure retriggers, not code changes. A duplicate-PR bot flagged #29709 covering the same fix; that's a merge-coordination concern, not a correctness one.
454bb69 to
a2535f9
Compare
There was a problem hiding this comment.
LGTM — minimal, correct fix; remaining inline comments are non-blocking (pre-existing / test nit).
Extended reasoning...
Overview
Two one-line additions: .JSX is added to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js. This enables the existing visited-map guard at the top of printAs for JSX-tagged values, turning a fuzzer-found stack overflow into a [Circular] print.
Security risks
None. This is output-formatting code; the change strictly adds a cycle check to a path that previously had none. No new attack surface, no auth/crypto/permissions involvement.
Level of scrutiny
Low. The mechanism (canHaveCircularReferences() gating a getOrPut/[Circular] guard) is established and already covers eight other tags in ConsoleObject.zig. I verified the .JSX branch in both formatters never re-dispatches printAs on the same value (only on type/key/props/children sub-values), so there is no spurious-[Circular] risk of the kind that forced the .Event revert earlier in this PR's history.
Other factors
All blocking feedback from earlier rounds has been addressed: the .Event regression I flagged was reverted in cdfc31b, and the test now drains stderr and asserts stdout before exitCode per repo conventions. The two unresolved inline comments are mine and explicitly non-blocking — one is a pre-existing props.getObject().? crash on the same path (out of scope, noted for follow-up), the other is a test nit about filtering the ASAN startup banner that CI will surface if it actually matters. Commits since my last review are CI retriggers only; the diff is unchanged. The bug hunting system found no issues this round.
a2535f9 to
d576cd9
Compare
There was a problem hiding this comment.
LGTM — the two-line fix is correct and the remaining open items are non-blocking nits / pre-existing issues.
Extended reasoning...
Overview
Adds .JSX to canHaveCircularReferences() in both src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, plus a subprocess regression test in test/js/bun/util/inspect.test.js. The Zig change is two enum-list additions; the test exercises three circular-JSX shapes (via key, props.children, and a custom prop) and asserts [Circular] output with exit code 0.
Security risks
None. This only widens the set of tags for which the existing visited-map / stack-depth guard runs in the inspect/snapshot formatters. No new I/O, parsing, auth, or untrusted-input surface.
Level of scrutiny
Low. The fix is the minimal, obvious change for a fuzzer-found stack overflow: include .JSX in the comptime predicate that gates the cycle check. I verified the .JSX printAs branch in both files never calls printAs(..., value, ...) on the same value (it only recurses on key, prop values, and children), so there is no spurious-[Circular] regression of the kind that forced reverting the .Event addition earlier in this PR's review.
Other factors
This PR has already been through several review rounds. The substantive feedback (the .Event regression, test assertion ordering / draining stderr) was addressed; the two open inline comments are (1) a pre-existing props.getObject().? crash on hand-crafted JSX that predates this PR, and (2) a test-ergonomics nit about filtering the ASAN stderr banner — neither blocks. Recent commits are purely CI retriggers for infrastructure outages with no code changes. A duplicate PR (#29709) independently arrived at the same fix.
0e2c521 to
9d3a291
Compare
There was a problem hiding this comment.
LGTM — two-line fix adding Tag::JSX to circular-reference detection, with a passing regression test; the other 10 .rs files are pure rustfmt whitespace from autofix.ci.
Extended reasoning...
Overview
The substantive change is two one-line additions: Tag::JSX is added to can_have_circular_references() in src/jsc/ConsoleObject.rs:2084 and src/runtime/test_runner/pretty_format.rs:530. This gates the visited-map / depth check at the top of print_as, so a JSX-like object whose key/props/children references itself now prints [Circular] instead of recursing until stack overflow. A regression test in inspect.test.js spawns a subprocess and asserts the exact output for three circular shapes. The remaining 10 .rs files (crash_handler, errno, tracy, Arguments, run_command, upgrade_command, jsc_hooks, ChromeProcess, spawn/process, spawn_sys/spawn_process) are 100% rustfmt reflow of long #[cfg(any(...))] / #[cfg_attr(...)] attributes onto multiple lines, applied by autofix.ci — zero semantic change.
Security risks
None. This is output formatting; no auth, crypto, fs, or network surface is touched. The fix removes a user-JS-reachable crash (Fuzzilli fingerprint bb8715595a137556).
Level of scrutiny
Low. Adding a tag to a matches! set that already contains Object/Array/Map/Set/etc. is the canonical fix for this bug class. I verified the .JSX arm dispatches to a dedicated print_jsx (ConsoleObject.rs:3617) and never re-enters print_as on the same value with a different tag, so it cannot hit the spurious-[Circular] regression that the briefly-added .Event did (now reverted per cdfc31b). The regression test's exact-string assertions (<div key=[Circular] /> etc.) independently confirm no false positives.
Other factors
All earlier review threads are resolved: the .Event add/revert cycle landed cleanly back at JSX-only scope; the test was updated to read/assert stderr and order exitCode last per repo conventions. My two remaining inline notes are explicitly non-blocking — one is a 🟣 pre-existing props.getObject() unwrap unrelated to this diff, and the 🟡 ASAN-banner filter nit was effectively mooted by CI (Build #55037 passed inspect.test.js on all platforms; the lone failure is an unrelated worker_threads SIGTRAP on macOS aarch64). A bot flagged #29709 as a possible duplicate, which is a merge-coordination concern rather than a correctness one.
When a React element's key, props, or children referenced the element itself, Bun.inspect would recurse infinitely and crash with a stack overflow. The JSX tag was not included in the set of tags that can have circular references, so the visited-map check was skipped. Add Tag::JSX to can_have_circular_references() in both the console formatter and the jest snapshot formatter.
fd69ece to
d8afc4d
Compare
There was a problem hiding this comment.
LGTM — one enum variant added to two matches! arms to enable existing cycle detection for JSX, with a solid regression test.
Extended reasoning...
Overview
The PR adds Tag::JSX to can_have_circular_references() in src/jsc/ConsoleObject.rs (one line in a matches! arm) and src/runtime/test_runner/pretty_format.rs (same), plus a subprocess regression test in test/js/bun/util/inspect.test.js covering three circular JSX shapes (via key, props.children, and an arbitrary prop).
Correctness
I traced both print_jsx / Tag::JSX print paths: every recursive self.format(...) call operates on a sub-value (key_value, property_value, children, child) — never on value itself. So unlike the .Event fall-through case that was tried and reverted earlier in this PR's history, enabling the visited-map for JSX cannot produce a spurious [Circular] on the element; it strictly turns an unbounded recursion into a bounded one for genuine cycles. The visited-map insert/remove gating at print_as_prelude (ConsoleObject.rs:3537) and the FORMAT.can_have_circular_references() guards (pretty_format.rs:1179/2644) already handle the bookkeeping.
Security risks
None. This is output-formatting cycle detection; it narrows behavior from crash → [Circular] string. No new inputs, no auth/permissions/data exposure surface.
Level of scrutiny
Low. Two-line mechanical change to enum match arms that opts JSX into pre-existing, well-exercised cycle-detection machinery used by Object/Array/Map/Set. The regression test locks in exact output for all three cycle entry points. No CODEOWNERS for these paths.
Other factors
All substantive prior feedback was addressed: the .Event addition that caused a fall-through regression was reverted (cdfc31b), and the test was updated to read/assert stderr and check exit code last. The one remaining unresolved inline comment is my own self-described nit about filtering the ASAN stderr banner — it's a test-ergonomics concern (cheap insurance against a possible linux-asan flake), not a correctness issue, and ~80 other tests in the repo use the same unfiltered expect(stderr).toBe("") pattern. CI has run on the final commit (d8afc4d, build #56858). The pre-existing props.getObject() hardening and .Event in pretty_format.rs were explicitly deferred to follow-ups and are out of scope here.
|
Closing as a duplicate of #29709. The source change is byte-identical: add |
1 participant
Inspecting a React element whose
key,props, orchildrenreferenced the element itself crashed with a stack overflow.The
JSXtag was not included incan_have_circular_references(), so the visited-map and stack checks inprint_aswere skipped for JSX elements. Circular JSX elements now print[Circular]like other circular values.Found by Fuzzilli (fingerprint
bb8715595a137556).What does this PR do?
Adds
Tag::JSXtocan_have_circular_references()insrc/jsc/ConsoleObject.rsandsrc/runtime/test_runner/pretty_format.rs.How did you verify your code works?
Regression test in
test/js/bun/util/inspect.test.jsspawns a subprocess that inspects three circular JSX shapes (viakey,props.children, and an arbitrary prop) and asserts[Circular]output with exit code 0. Without the fix, the subprocess segfaults (exit 139) and the test fails.