Skip to content

Fix stack overflow in Bun.inspect for circular JSX elements #30126

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
+31 −2
Closed

Fix stack overflow in Bun.inspect for circular JSX elements #30126

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
1 change: 1 addition & 0 deletions src/jsc/ConsoleObject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2089,6 +2089,7 @@ pub mod formatter {
| Tag::Error
| Tag::Class
| Tag::Event
| Tag::JSX
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/test_runner/pretty_format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ impl Tag {

#[inline]
pub const fn can_have_circular_references(self) -> bool {
matches!(self, Tag::Array | Tag::Object | Tag::Map | Tag::Set)
matches!(self, Tag::Array | Tag::Object | Tag::Map | Tag::Set | Tag::JSX)
}
}

Expand Down
30 changes: 29 additions & 1 deletion test/js/bun/util/inspect.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from "bun:test";
import { normalizeBunSnapshot, tmpdirSync } from "harness";
import { bunEnv, bunExe, normalizeBunSnapshot, tmpdirSync } from "harness";
import { join } from "path";
import util from "util";
it("prototype", () => {
Expand Down Expand Up @@ -316,6 +316,34 @@ it("jsx with fragment", () => {
expect(input).toBe(output);
});

it("jsx with circular references does not stack overflow", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const sym = Symbol.for("react.element");
const a = { $$typeof: sym, type: "div", key: null, props: {} };
a.key = a;
console.log(Bun.inspect(a));
const b = { $$typeof: sym, type: "div", key: null, props: {} };
b.props.children = b;
console.log(Bun.inspect(b));
const c = { $$typeof: sym, type: "div", key: null, props: {} };
c.props.foo = c;
console.log(Bun.inspect(c));
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
Comment on lines +341 to +342

@claude claude Bot May 3, 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.

🟡 🟡 Nit: CodeRabbit's suggestion you partially applied in 61823f2 also filtered the WARNING: ASAN interferes with JSC signal handlers… banner that ASAN builds write to stderr at startup (see expectations.txt:15 and the 17 sibling subprocess tests — e.g. string_decoder.test.js:303-307, setTimeout.test.js:429 — that strip it before asserting empty stderr). With a raw expect(stderr).toBe("") this test may go red on the linux-asan job; the one-line filter is cheap insurance:

expect(stderr.split("\n").filter(l => l && !l.startsWith("WARNING: ASAN interferes"))).toEqual([]);
Extended reasoning...

What this is

Commit 61823f2 addressed the earlier review by reading proc.stderr.text() and reordering the assertions, but dropped one piece of CodeRabbit's committable suggestion (inline comment 3177287793): the filter for the ASAN startup banner. On linux ASAN builds, the bun-asan binary may write WARNING: ASAN interferes with JSC signal handlers; … to stderr during JSC initialization (the comment at string_decoder.test.js:303 attributes it to WebKit's Options.cpp, which is not in this checkout). bunEnv (test/harness.ts:72-74) sets ASAN_OPTIONS=allow_user_segv_handler=1:disable_coredump=0, which does not suppress that banner. With the new expect(stderr).toBe("") at line 342, the linux-asan job would see received: "WARNING: ASAN interferes with JSC signal handlers; …\n" and fail this test.

Why existing safeguards don't prevent it

There is no harness helper that strips this line — every subprocess test that wants to assert empty stderr under ASAN does it inline. Seventeen test files in the repo currently filter exactly this prefix (e.g. test/js/node/string_decoder/string-decoder.test.js:305-308, test/js/web/timers/setTimeout.test.js:429, test/js/bun/resolve/bun-main-entry-point.test.ts:16, test/js/node/buffer-copy-fill-detach.test.ts:23, test/js/bun/udp/udp_socket.test.ts:39, test/regression/issue/29519.test.ts), several with a verbatim comment that "ASAN builds unconditionally print … to stderr at startup". And test/expectations.txt:15 skips child_process.test.ts on ASAN with the note # Unexpected identifier "WARNING" — independent evidence that the banner is real on the ASAN builder, not just a CodeRabbit-learned cargo-cult.

Step-by-step proof

  1. linux-asan job spawns bunExe() (= bun-asan) with -e <script> and env: bunEnv.
  2. During JSC Options::initialize(), the ASAN-aware path emits WARNING: ASAN interferes with JSC signal handlers; …\n to stderr before any user code runs.
  3. The script runs, writes the three <div …> lines to stdout, exits 0.
  4. proc.stderr.text() resolves to "WARNING: ASAN interferes with JSC signal handlers; …\n".
  5. expect(stderr).toBe("") fails; the stdout/exitCode assertions never run.

Addressing the refutation

One reviewer points out that the literal fprintf is not in src/ (only the comment at ZigGlobalObject.cpp:288), that test/CLAUDE.md's canonical spawn pattern is the unfiltered expect(stderr).toBe(""), and that ~80 other tests use that unfiltered form without being listed as ASAN failures in expectations.txt. That's all accurate, and it's why this is a nit rather than a guaranteed CI break: either the banner is gated on something not every test hits, or the linux-asan job tolerates/curates these. But the expectations.txt skip (Unexpected identifier "WARNING") and the seventeen explicit filters — including one merged the day before this PR in #30058 — are direct evidence that the banner does surface on the ASAN builder for at least some bunExe() spawns. Since CodeRabbit already proposed the filter on this exact diff and the author applied two-thirds of that suggestion, finishing it is the path of least resistance.

Fix

const stderrLines = stderr.split("\n").filter(l => l && !l.startsWith("WARNING: ASAN interferes"));
expect(stderrLines).toEqual([]);

(or the two-.filter() form from CodeRabbit's committable suggestion). Test-only, no behavior change on non-ASAN builds.

expect(stdout).toBe("<div key=[Circular] />\n" + "<div>\n [Circular]\n</div>\n" + "<div foo=[Circular] />\n");
expect(exitCode).toBe(0);
});

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