Fix stack overflow when inspecting circular JSX elements#30657
Conversation
Bun.inspect / console.log would recurse without bound when a React element contained a reference to itself (via key, props, or children), leading to a segfault. Include .JSX in canHaveCircularReferences so the visited-map and stack-depth guards apply.
|
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 (3)
WalkthroughThe PR extends circular-reference detection to JSX values by marking the 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 |
|
Duplicate of #29709. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward crash fix that enables the existing circular-reference guard for the JSX formatter tag.
Extended reasoning...
Overview
Two one-line changes adding .JSX to canHaveCircularReferences() in src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig, plus four regression tests in test/js/bun/util/inspect.test.js. The .JSX branch of printAs() recurses into key / props / children via this.format(), but was excluded from the comptime-gated visited-map check at the top of printAs(), so a self-referential React element exhausted the native stack. Adding the tag enables the same getOrPut / [Circular] short-circuit already used for .Array, .Object, .Map, .Set.
Security risks
None. This is purely additive to a debug/inspection code path — it turns a SIGSEGV into a [Circular] string. No new inputs are read, no new control flow is reachable, no auth/crypto/permissions involved.
Level of scrutiny
Low. The fix is mechanical and mirrors the existing pattern for every other recursive tag; the only behavior change is that the visited-map guard now fires for JSX values. The four new tests cover all recursion entry points in the .JSX arm (key, arbitrary prop, children as JSX, children as array).
Other factors
The inline note about .Event still being absent from pretty_format.zig's version is a pre-existing, contrived edge case (requires overriding a native getter with defineProperty) and is explicitly flagged as a non-blocking parity nit. It doesn't affect the correctness of this fix.
| pub inline fn canHaveCircularReferences(tag: Tag) bool { | ||
| return tag == .Array or tag == .Object or tag == .Map or tag == .Set; | ||
| return tag == .Array or tag == .Object or tag == .Map or tag == .Set or tag == .JSX; | ||
| } |
There was a problem hiding this comment.
🟣 While you're on this line: ConsoleObject.zig's canHaveCircularReferences() also includes .Event, but this version still doesn't. The .Event branch in printAs() here recurses via this.format() into a MessageEvent's data / ErrorEvent's error, so an event whose data points back to itself can still stack-overflow the Jest formatter — same root cause as the JSX fix. Pre-existing and contrived, but it's a one-token addition for parity.
Extended reasoning...
What
This PR adds .JSX to canHaveCircularReferences() in both ConsoleObject.zig and pretty_format.zig so that circular JSX elements get a visited-map guard. However, the two functions are still not in parity: ConsoleObject.zig:1139 lists .Function, .Array, .Object, .Map, .Set, .Error, .Class, .Event, .JSX, while pretty_format.zig:329 after this PR lists only .Array, .Object, .Map, .Set, .JSX. The missing one that matters is .Event — the .Event branch in pretty_format.zig's printAs() does recurse into user-controlled values, so it has the exact same stack-overflow exposure that this PR is fixing for .JSX. (.Function/.Class/.Error in pretty_format.zig do not recurse, so they don't need to be added — .Event is the only real gap.)
Code path
MessageEvent,ErrorEvent,CloseEventetc. are constructed withJSType(JSEventType)(0b11101111) persrc/jsc/bindings/JSDOMWrapper.h, which maps toJSType.Eventin Zig. InTag.get(), this is distinct from.DOMWrapper(which returns.Private), so it falls through to the final switch and returns.tag = .Event.format()dispatches toprintAs(.Event, ...). Because.Eventis not incanHaveCircularReferences(), theif (comptime Format.canHaveCircularReferences())block is skipped — no entry is inserted intothis.mapfor the event.- Inside the
.Event => { ... }arm, for aMessageEventthe formatter doesconst data = (try value.fastGet(this.globalThis, .data)) orelse .js_undefined;and thentry this.format(tag, ..., data, ...). For anErrorEventit does the same with.error.fastGetis a normal property lookup, so an own-property override is honored. - If
data(orerror) is the event itself,Tag.get(data)returns.Eventagain, and step 2 re-entersprintAs(.Event, value)with no visited-map check anywhere on the path.
Why nothing else catches it
The visited-map guard in printAs is gated entirely on comptime Format.canHaveCircularReferences(). Since .Event returns false there, the getOrPut / found_existing check never runs for the event value. The only other recursion limiter in this file is the per-type guard, and there is none for .Event. Note that indirect cycles through a plain object (e.g. ev.data = obj; obj.x = ev) are caught, because .Object is guarded and inserts obj into the map — only the direct Event→Event self-reference loops forever.
Step-by-step proof
const ev = new MessageEvent("message");
Object.defineProperty(ev, "data", { value: ev, configurable: true });
// In the Jest pretty-formatter (e.g. via expect(ev).toMatchSnapshot()):
// Tag.get(ev) -> { tag: .Event }
// printAs(.Event, ev) -> canHaveCircularReferences() == false, no map insert
// fastGet(ev, .data) -> ev
// Tag.get(ev) -> { tag: .Event }
// format -> printAs(.Event, ev) // again, no map insert
// fastGet(ev, .data) -> ev // ...
// Native stack overflow.The Object.defineProperty override is the same class of "contrived but fuzzer-reachable" input as the JSX repro this PR fixes (and inspect.test.js already exercises Object.defineProperty on MessageEvent for the "deleted data" test, so it's a supported shape).
Impact and fix
Impact: a snapshot/diff of such an event in bun test crashes the process with a native stack overflow instead of printing [Circular]. It's pre-existing and requires deliberately overriding a native getter, so it shouldn't block this PR — but since the fix is literally adding or tag == .Event to the line already being edited, and ConsoleObject.zig already has it, it's worth doing here for parity.
1 participant
What
Bun.inspect/console.logsegfaulted with a stack overflow when a React element contained a reference to itself viakey,props, orchildren.Why
The
.JSXformatter tag was not included incanHaveCircularReferences(), so the visited-map and stack-depth guards were skipped. The formatter recursed intokey/props/children, hit the same element, and looped until the native stack was exhausted.Fix
Add
.JSXtocanHaveCircularReferences()in bothConsoleObject.zigandpretty_format.zig. Circular JSX now prints[Circular]like other object types.Found by Fuzzilli (fingerprint
471aa8ea6d631cd1).