Fix stack overflow when inspecting circular JSX elements#30021
Conversation
Bun.inspect() and console.log() on a React element whose key, props, or children reference the element itself would recurse unboundedly and crash with a stack overflow. Add .JSX to canHaveCircularReferences() so the existing visited-set and stack guard apply, printing [Circular] instead.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe circular-reference detection logic in the formatter is updated to treat React JSX elements as safely recurseable values, enabling consistent circular-reference tracking for JSX alongside other container types. Corresponding tests validate the behavior with self-referential JSX element structures. 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: 0/5 reviews remaining, refill in 55 minutes and 37 seconds. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29709 which covers this plus the same bug in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the one-line fix and tests are correct for Bun.inspect; left a note about the parallel gap in pretty_format.zig which you may want to fold in here or do as a follow-up.
Extended reasoning...
Overview
This PR adds .JSX to the canHaveCircularReferences() switch in src/bun.js/ConsoleObject.zig so that the visited-set [Circular] guard and stack_check.isSafeToRecurse() fallback apply when formatting React elements. Four regression tests in test/js/bun/util/inspect.test.js cover circular key, circular prop, circular children, and a non-circular repeated-child case to guard against false positives.
Security risks
None. This is a defensive change to a debug/inspection formatter; it strictly adds a recursion guard. No auth, crypto, parsing, or untrusted-input handling is touched.
Level of scrutiny
Low. The production change is a single enum tag added to an existing switch arm whose semantics ("this tag's value can contain itself") are obviously true for JSX elements (objects with arbitrary key/props/children). The surrounding machinery (visited map insert/remove, stack check) is unchanged and already exercised by the other tags. The new tests verify both the crash fix and that the visited-set doesn't over-trigger on repeated-but-acyclic children.
Other factors
The bug-hunting system correctly flagged that the sister formatter JestPrettyFormat (src/bun.js/test/pretty_format.zig:328-330) has the same omission, so a circular JSX element reaching expect() diff output can still overflow. That is a pre-existing issue in a file this PR does not touch, not a defect in the change itself, so I'm treating it as a non-blocking follow-up rather than a reason to withhold approval. The fix as scoped — Bun.inspect / console.log — is complete and well-tested.
| pub fn canHaveCircularReferences(tag: Tag) bool { | ||
| return switch (tag) { | ||
| .Function, .Array, .Object, .Map, .Set, .Error, .Class, .Event => true, | ||
| .Function, .Array, .Object, .Map, .Set, .Error, .Class, .Event, .JSX => true, |
There was a problem hiding this comment.
🔴 The same fix is needed in the sister formatter src/bun.js/test/pretty_format.zig — its canHaveCircularReferences() (lines 328-330) still omits .JSX, so a circular React element passed to a failing expect() matcher (e.g. expect(el).toBe(null)) still recurses unboundedly in DiffFormatter → JestPrettyFormat and SIGSEGVs. Mirroring this one-line change there (and ideally also adding .Error/.Event/.Function/.Class for parity) would close the remaining hole.
Extended reasoning...
Summary
This PR fixes the circular-JSX stack overflow in Bun.inspect()/console.log() by adding .JSX to canHaveCircularReferences() in src/bun.js/ConsoleObject.zig. However, Bun has a near-identical second formatter — JestPrettyFormat in src/bun.js/test/pretty_format.zig — with its own copy of canHaveCircularReferences() that was not updated. That copy still only returns true for .Array, .Object, .Map, and .Set, so the exact crash this PR's title promises to fix is still reachable through expect() failure output.
Code path
JestPrettyFormat is invoked from user code via DiffFormatter (src/bun.js/test/diff_format.zig:39/48), which renders the "Expected/Received" diff for failing matchers like toBe, toEqual, toStrictEqual, toHaveProperty, toMatchSnapshot, etc. When the received value is a React element, Tag.get() returns .JSX and printAs(.JSX, ...) runs.
Inside the .JSX branch (pretty_format.zig:1477-1700):
- line 1531 calls
this.format(...)on the element'skey - line 1580 calls
this.format(...)on each prop value - lines 1643/1666 call
this.format(...)onchildren
The visited-map [Circular] short-circuit at lines 876-888 is gated by if (comptime Format.canHaveCircularReferences()). Since .JSX returns false from canHaveCircularReferences() in this file, that guard is compiled out for the JSX path. Additionally, unlike ConsoleObject.zig, pretty_format.zig has no stack_check.isSafeToRecurse() fallback anywhere in the file, so there is no secondary depth limit either.
Step-by-step proof
const el = { $$typeof: Symbol.for('react.element'), type: 'div', props: {}, key: null };
el.key = el;
expect(el).toBe(null); // fails by referencetoBefails →DiffFormatteris asked to formatreceived = el.diff_format.zig:39callsJestPrettyFormat.format(..., el, ...).Tag.get(el)sees$$typeof === Symbol.for('react.element')→ returns.JSX.format()enters theif (comptime Format.canHaveCircularReferences())block — skipped, because for.JSXit isfalsein pretty_format.zig.printAs(.JSX, ...)reaches line 1531 and callsthis.format(...)onel.key, which iselitself.- Goto step 3. No visited-set entry was ever recorded, no stack guard exists → unbounded recursion → SIGSEGV.
The same applies if the cycle goes through props.foo = el or props.children = el.
Impact
Any test that asserts on a self-referencing React element (or any object graph that contains one) and fails will crash the test runner with a segfault instead of printing a diff. This is the exact crash class described in the PR title and description, just on the bun:test formatting path rather than the Bun.inspect path.
Fix
Mirror the change in src/bun.js/test/pretty_format.zig:329:
.Array, .Object, .Map, .Set, .JSX => true,For full parity with ConsoleObject.zig it would also make sense to add .Error, .Event, .Function, .Class while you're there, since that file's canHaveCircularReferences() lags behind on those tags too.
1 participant
What does this PR do?
Bun.inspect()/console.log()on a React element whosekey,props, orchildrenreference the element itself would recurse unboundedly and crash with a stack overflow (SIGSEGV).The
.JSXformatter tag was missing fromcanHaveCircularReferences(), so the visited-set[Circular]short-circuit and thestack_check.isSafeToRecurse()guard were skipped for JSX elements.How did you verify your code works?
Added regression tests covering circular
key, circularprops, circularchildren, and repeated (non-circular) children to ensure those still print normally.Found by Fuzzilli, fingerprint
bb8715595a137556.