Skip to content

Fix stack overflow when inspecting circular JSX elements #30657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

Fix stack overflow when inspecting circular JSX elements #30657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/jsc/ConsoleObject.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,7 +1136,7 @@ pub const Formatter = struct {

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,
else => false,
};
}
Expand Down
2 changes: 1 addition & 1 deletion src/test_runner/pretty_format.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,9 @@
};
}

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;
}

Check notice on line 330 in src/test_runner/pretty_format.zig

View check run for this annotation

Claude / Claude Code Review

pretty_format.zig: .Event missing from canHaveCircularReferences() 

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.
Comment on lines 328 to 330

@claude claude Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 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

  1. MessageEvent, ErrorEvent, CloseEvent etc. are constructed with JSType(JSEventType) (0b11101111) per src/jsc/bindings/JSDOMWrapper.h, which maps to JSType.Event in Zig. In Tag.get(), this is distinct from .DOMWrapper (which returns .Private), so it falls through to the final switch and returns .tag = .Event.
  2. format() dispatches to printAs(.Event, ...). Because .Event is not in canHaveCircularReferences(), the if (comptime Format.canHaveCircularReferences()) block is skipped — no entry is inserted into this.map for the event.
  3. Inside the .Event => { ... } arm, for a MessageEvent the formatter does const data = (try value.fastGet(this.globalThis, .data)) orelse .js_undefined; and then try this.format(tag, ..., data, ...). For an ErrorEvent it does the same with .error. fastGet is a normal property lookup, so an own-property override is honored.
  4. If data (or error) is the event itself, Tag.get(data) returns .Event again, and step 2 re-enters printAs(.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.


const Result = struct {
tag: Tag,
Expand Down
18 changes: 18 additions & 0 deletions test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,24 @@ it("jsx with fragment", () => {
expect(input).toBe(output);
});

it("jsx with circular reference", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props: {} };
el.key = el;
expect(Bun.inspect(el)).toBe("<div key=[Circular] />");

const el2 = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props: {} };
el2.props.foo = el2;
expect(Bun.inspect(el2)).toBe("<div foo=[Circular] />");

const el3 = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props: {} };
el3.props.children = el3;
expect(Bun.inspect(el3)).toBe("<div>\n [Circular]\n</div>");

const el4 = { $$typeof: Symbol.for("react.element"), type: "div", key: null, ref: null, props: {} };
el4.props.children = [el4];
expect(Bun.inspect(el4)).toBe("<div>\n [Circular]\n</div>");
});

it("inspect", () => {
expect(Bun.inspect(new TypeError("what")).includes("TypeError: what")).toBe(true);
expect(Bun.inspect("hi")).toBe('"hi"');
Expand Down
Loading