Fix stack overflow when inspecting JSX elements with circular references#29931
Fix stack overflow when inspecting JSX elements with circular references#29931robobun wants to merge 2 commits into
Conversation
Bun.inspect() on a React element whose props (or children) referenced the element itself would recurse until the native stack was exhausted, because the .JSX format tag was not included in canHaveCircularReferences() and therefore skipped both the visited-map check and the stack-overflow guard. Also fix a panic when props is not an object.
|
Updated 2:38 AM PT - Apr 29th, 2026
❌ @robobun, your commit 1463362 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29931That installs a local version of the PR into your bun-29931 --bun |
|
Caution Review failedPull request was closed or merged during review WalkthroughJSX inspection and pretty-printing were made more defensive: JSX-tagged nodes are now eligible for circular-reference detection, props extraction is conditional to avoid forced unwrapping, and tests were added to verify handling of circular JSX structures and non-object props. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 27 minutes and 30 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/ConsoleObject.zig`:
- Around line 3143-3148: The unsafe optional unwrap on props in
pretty_format.zig (the use of props.? causing a panic when props is a primitive)
should be replaced with the same safe pattern used in ConsoleObject.zig: call
props.getObject() and handle the non-object case with orelse break :props so you
don't panic; locate the props usage in the pretty_format function (around where
props is read and converted) and change the unwrap to use props.getObject()
orelse break :props to guard against primitive props.
🪄 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: 8e58ded4-8a9d-4624-8122-804562f83ab3
📒 Files selected for processing (2)
src/bun.js/ConsoleObject.zigtest/js/bun/util/inspect.test.js
|
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}
The expect() diff formatter has a duplicate JSX printing path with the same unsafe props.getObject().? unwrap and the same missing .JSX entry in canHaveCircularReferences(). Apply the identical fix and add a regression test that exercises pretty_format via expect().toEqual().
|
Duplicate of #29709 — same source fix, and that PR has more complete test coverage (circular key case) placed alongside the other JSX inspect tests. Closing in favor of it. |
There was a problem hiding this comment.
LGTM — thanks for mirroring the fix to pretty_format.zig and adding the expect().toEqual() regression test.
Extended reasoning...
Overview
Two near-identical, minimal Zig changes applied to both formatters (src/bun.js/ConsoleObject.zig and src/bun.js/test/pretty_format.zig):
- Add
.JSXtocanHaveCircularReferences()so the visited-map /[Circular]guard runs for JSX-tagged values. - Replace
props.getObject().?with a labeled-blockorelse break :propsso a non-objectpropsskips the props block instead of panicking.
Plus regression tests covering both code paths: Bun.inspect() in inspect.test.js and JestPrettyFormat (via a spawned expect().toEqual() failure diff) in pretty-format-overflow.test.ts.
Security risks
None. This is purely output-formatting / crash-hardening for the inspector; no auth, permissions, parsing of untrusted input beyond what already flows through the formatter, or data exposure.
Level of scrutiny
Low. The diff is ~6 lines of logic per formatter and follows the exact pattern already used for .Array/.Object/.Map/.Set. The break :props early-exit correctly runs the preceding defer to restore quote_strings, so no state leaks. My previous review's only blocking concern (the unfixed pretty_format.zig duplicate) was addressed in 1463362 with a dedicated regression test.
Other factors
- The one remaining finding is a nit about asserting
stderrbeforeexitCodein the new test for better failure diagnostics — it matches the ordering of the pre-existing test in the same file and doesn't affect correctness. - A duplicate-PR bot flagged #29709 as overlapping; that's a process decision for the maintainer (this PR appears more complete since it covers both formatters), not a correctness concern with this change.
| expect(exitCode).toBe(1); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("SIGSEGV"); | ||
| expect(stderr).toContain("[Circular]"); | ||
| expect(stderr).toContain("expect(received).toEqual(expected)"); | ||
| expect(stderr).toContain("3 fail"); |
There was a problem hiding this comment.
🟡 nit: per root CLAUDE.md, when spawning processes the test should assert on stderr content before exitCode so that a regression surfaces useful diagnostics. If the spawned bun test segfaults again (the very thing this test guards against), exitCode will be null/signal-coded and the failure will just say "expected null to be 1" without showing what stderr contained. Move expect(exitCode).toBe(1) after the stderr assertions (the pre-existing test above has the same ordering and could be flipped too).
Extended reasoning...
What the bug is
The new test in test/js/bun/test/pretty-format-overflow.test.ts asserts expect(exitCode).toBe(1) on line 88, before the stderr content assertions on lines 89-93 (not.toContain("panic"), not.toContain("SIGSEGV"), toContain("[Circular]"), etc.). Root CLAUDE.md explicitly says:
When spawning processes, tests should
expect(stdout).toBe(...)BEFOREexpect(exitCode).toBe(0). This gives you a more useful error message on test failure.
How it manifests / step-by-step
This test exists to catch a regression where JestPrettyFormat stack-overflows or panics on circular/non-object JSX props. Suppose that regression is reintroduced:
- The spawned
bun test jsx.test.tsprocess hits the native stack overflow / panic. - The child process is killed by a signal (e.g. SIGSEGV / SIGTRAP), so
proc.exitedresolves tonull(or a non-1signal exit code), andstderrcontains the crash banner instead of the expected diff output. - The first assertion executed is
expect(exitCode).toBe(1)on line 88. It fails with a message likeexpected null to be 1. - Execution stops there — none of the subsequent stderr assertions run, so the test report never shows the actual stderr content (which would have made the failure cause obvious:
panic: ...orSIGSEGV).
Why existing code doesn't prevent it
expect() failures throw immediately in bun:test, so once the exitCode assertion fails, the more informative stderr checks are skipped. Nothing else in the test prints stderr on failure.
Impact
Purely a test-ergonomics / diagnostics issue — the test still correctly fails on regression, it just produces a much less helpful failure message. Given this PR is specifically guarding against a crash, having the failure surface the crash output is valuable. This is nit-level: it doesn't affect correctness, and it matches the ordering already used by the pre-existing "deeply nested object" test in the same file (lines 46-51).
How to fix
Reorder the assertions so the stderr content checks run first and the exitCode check runs last:
expect(stderr).not.toContain("panic");
expect(stderr).not.toContain("SIGSEGV");
expect(stderr).toContain("[Circular]");
expect(stderr).toContain("expect(received).toEqual(expected)");
expect(stderr).toContain("3 fail");
expect(exitCode).toBe(1);Optionally apply the same reordering to the pre-existing test above for consistency.
1 participant
What
Bun.inspect()/console.log()crashed with a native stack overflow when passed a React element that contained a circular reference throughpropsorprops.children.Why
The formatter's
.JSXtag was not listed incanHaveCircularReferences(), so when formatting a JSX element it skipped both the visited-map check and theisSafeToRecurse()stack guard. With a self-referentialprops, the formatter recursed into the same element forever until the native stack was exhausted.While here, also fixed a
getObject().?null-unwrap panic whenpropsis a primitive instead of an object — we now skip the props block gracefully.How
.JSXtocanHaveCircularReferences()so circular JSX structures print[Circular]instead of crashing.props.getObject().?withprops.getObject() orelse break :props.Found by Fuzzilli (fingerprint
13aabad674c61341).