Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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/bun.js/ConsoleObject.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1136,7 +1136,7 @@

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,

Check failure on line 1139 in src/bun.js/ConsoleObject.zig

View check run for this annotation

Claude / Claude Code Review

Same circular-JSX stack overflow remains in JestPrettyFormat (expect() diff output)

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.

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.

🔴 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 DiffFormatterJestPrettyFormat 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's key
  • line 1580 calls this.format(...) on each prop value
  • lines 1643/1666 call this.format(...) on children

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 reference
  1. toBe fails → DiffFormatter is asked to format received = el.
  2. diff_format.zig:39 calls JestPrettyFormat.format(..., el, ...).
  3. Tag.get(el) sees $$typeof === Symbol.for('react.element') → returns .JSX.
  4. format() enters the if (comptime Format.canHaveCircularReferences()) block — skipped, because for .JSX it is false in pretty_format.zig.
  5. printAs(.JSX, ...) reaches line 1531 and calls this.format(...) on el.key, which is el itself.
  6. 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.

else => false,
};
}
Expand Down
31 changes: 31 additions & 0 deletions test/js/bun/util/inspect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -772,3 +772,34 @@ it("CustomEvent", () => {
}"
`);
});

describe("JSX circular references", () => {
it("does not crash when key is a circular reference", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: {}, key: null };
el.key = el;
expect(Bun.inspect(el)).toBe("<div key=[Circular] />");
});

it("does not crash when a prop is a circular reference", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: {}, key: null };
el.props.foo = el;
expect(Bun.inspect(el)).toBe("<div foo=[Circular] />");
});

it("does not crash when children is a circular reference", () => {
const el = { $$typeof: Symbol.for("react.element"), type: "div", props: {}, key: null };
el.props.children = el;
expect(Bun.inspect(el)).toBe("<div>\n [Circular]\n</div>");
});

it("does not mark repeated non-circular children as circular", () => {
const child = { $$typeof: Symbol.for("react.element"), type: "span", props: {}, key: null };
const parent = {
$$typeof: Symbol.for("react.element"),
type: "div",
props: { children: [child, child] },
key: null,
};
expect(Bun.inspect(parent)).toBe("<div>\n <span />\n <span />\n</div>");
});
});
Loading