Fix crash when inspecting JSX elements with circular or non-object props - #30630
Closed
robobun wants to merge 1 commit into
Closed
Fix crash when inspecting JSX elements with circular or non-object props#30630robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Conversation
The JSX tag was not included in canHaveCircularReferences, so inspecting a React element whose props/key/children referred back to itself would recurse until the stack overflowed. The JSX formatter also unconditionally unwrapped props as an object, panicking when props was a primitive.
Collaborator
Author
|
Updated 11:18 AM PT - May 13th, 2026
❌ @robobun, your commit dbf0c2c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30630That installs a local version of the PR into your bun-30630 --bun |
Contributor
WalkthroughThis PR extends JSX circular-reference detection in two formatting implementations ( ChangesJSX Circular Reference Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test_runner/pretty_format.zig`:
- Around line 329-330: The pretty formatter currently doesn't treat React 19
transitional elements as JSX because the JSX detection condition only checks
tags (.Array, .Object, .Map, .Set, .JSX); update the JSX detection logic in
src/test_runner/pretty_format.zig to also recognize
Symbol.for("react.transitional.element") the same way ConsoleObject.zig does
(mirror the transitional symbol check used around the ConsoleObject handling) so
that when the inspected value matches the transitional element symbol it will
return tag == .JSX and thus participate in the pretty printer's circular
reference/JSX handling; locate the JSX/tag-checking function (the block that
returns tag == .Array or tag == .Object or tag == .Map or tag == .Set or tag ==
.JSX) and add the transitional symbol check analogous to ConsoleObject.zig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d223d7e-09d6-4a54-a0d4-32e49eb3d03d
📒 Files selected for processing (3)
src/jsc/ConsoleObject.zigsrc/test_runner/pretty_format.zigtest/js/bun/util/inspect.test.js
Comment on lines
+329
to
330
| return tag == .Array or tag == .Object or tag == .Map or tag == .Set or tag == .JSX; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking JSX symbol handling parity between inspect and pretty formatter..."
rg -n -C2 'react\.element|react\.transitional\.element|react\.fragment|\$\$typeof' src/jsc/ConsoleObject.zig src/test_runner/pretty_format.zig
echo
echo "Checking tests for transitional symbol coverage..."
rg -n -C2 'react\.transitional\.element|react\.element|\\$\\$typeof' testRepository: oven-sh/bun
Length of output: 8294
Add react.transitional.element support to pretty formatter for React 19 parity
pretty_format.zig doesn't recognize Symbol.for("react.transitional.element"), creating a parity gap with ConsoleObject.zig which already handles it. This prevents React 19 transitional elements from being tagged as .JSX in Jest pretty formatting, bypassing circular reference handling for these elements.
Mirror the approach in ConsoleObject.zig (lines 1305-1310) by adding the transitional symbol check:
Fix
var reactElement = ZigString.init("react.element");
+ var react_element_transitional = ZigString.init("react.transitional.element");
var react_fragment = ZigString.init("react.fragment");
- if (try typeof_symbol.isSameValue(.symbolFor(globalThis, &reactElement), globalThis) or try typeof_symbol.isSameValue(.symbolFor(globalThis, &react_fragment), globalThis)) {
+ if (try typeof_symbol.isSameValue(.symbolFor(globalThis, &reactElement), globalThis) or
+ try typeof_symbol.isSameValue(.symbolFor(globalThis, &react_element_transitional), globalThis) or
+ try typeof_symbol.isSameValue(.symbolFor(globalThis, &react_fragment), globalThis))
+ {
return .{ .tag = .JSX, .cell = js_type };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test_runner/pretty_format.zig` around lines 329 - 330, The pretty
formatter currently doesn't treat React 19 transitional elements as JSX because
the JSX detection condition only checks tags (.Array, .Object, .Map, .Set,
.JSX); update the JSX detection logic in src/test_runner/pretty_format.zig to
also recognize Symbol.for("react.transitional.element") the same way
ConsoleObject.zig does (mirror the transitional symbol check used around the
ConsoleObject handling) so that when the inspected value matches the
transitional element symbol it will return tag == .JSX and thus participate in
the pretty printer's circular reference/JSX handling; locate the
JSX/tag-checking function (the block that returns tag == .Array or tag ==
.Object or tag == .Map or tag == .Set or tag == .JSX) and add the transitional
symbol check analogous to ConsoleObject.zig.
Collaborator
Author
There was a problem hiding this comment.
This is a pre-existing parity gap unrelated to the crash being fixed here. When pretty_format.zig doesn't recognize react.transitional.element, it falls through to .Object, which already participates in circular reference detection — so there's no crash risk for that case. Keeping this PR scoped to the crash fix.
Contributor
There was a problem hiding this comment.
@robobun, that makes sense — since unrecognized react.transitional.element objects fall through to .Object, which is already in canHaveCircularReferences(), there's no crash exposure for that case. Keeping the PR scoped to the crash fix is the right call. Thanks for the clarification!
🧠 Learnings used
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 26310
File: src/cache.zig:123-127
Timestamp: 2026-01-20T21:51:21.482Z
Learning: Ensure readFileShared in Zig files follows the same FD lifecycle as readFileWithAllocator: compute will_close = rfs.needToCloseFiles() and opened_file; use it in defer; and return bun.invalid_fd (not the closed handle) when will_close is true to avoid exposing closed FDs. This pattern should be applied to similar readFile* helpers across the Zig codebase to maintain correct resource lifecycle and avoid leaking or returning invalid file descriptors.
Learnt from: jimmywarting
Repo: oven-sh/bun PR: 26595
File: src/bun.js/webcore/Blob.zig:3839-3841
Timestamp: 2026-02-12T22:09:40.373Z
Learning: In Zig source files implementing Blob (e.g., Blob constructors in src/bun.js/webcore/Blob.zig), ensure that Blob construction does not synchronously read file contents. When building a Blob from parts that include file-backed blobs (e.g., Bun.file() results), create references to those blobs that preserve offset/size metadata instead of eagerly loading bytes into memory. Only the read methods (text(), arrayBuffer(), stream(), etc.) should perform actual file I/O. This aligns with Web API semantics (Blob constructor is synchronous but non-blocking) and common runtime patterns (Node.js, Deno, fetch-blob). The Blob should internally store an array of BlobParts (ArrayBuffer, file-backed Blob with offset/size, other Blob references) and materialize bytes only when read methods are invoked.
Learnt from: alii
Repo: oven-sh/bun PR: 28128
File: src/cli/test/Scanner.zig:170-178
Timestamp: 2026-03-15T05:22:35.951Z
Learning: In oven-sh/bun's Zig code, avoid flagging small fixed-size buffers as needing path_buffer_pool. Use path_buffer_pool.get()/put() only when you actually need a MAX_PATH_BYTES-sized buffer (around ~96KB on Windows). For small buffers (e.g., [4096]u8) used for short relative paths under ~1KB, allocate on the stack and avoid pool overhead on hot paths. Do not apply path_buffer_pool guidance unless a full MAX_PATH_BYTES-sized buffer is required.
Learnt from: robobun
Repo: oven-sh/bun PR: 28299
File: src/bun.js/api/cron.zig:311-315
Timestamp: 2026-03-20T16:27:36.818Z
Learning: In oven-sh/bun Zig code, treat these as equivalent and acceptable: `bun.FD.cwd().makePath(u8, path)` and `bun.makePath(bun.FD.cwd().stdDir(), path)`. The underlying implementation (`FD.makePath` around src/fd.zig) is a thin wrapper that delegates to `bun.makePath`, so reviewers should not flag `bun.FD.cwd().makePath(u8, ...)` as non-standard or incorrect when it’s used for path creation.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 28611
File: src/http.zig:565-620
Timestamp: 2026-03-30T19:17:01.758Z
Learning: In this oven-sh/bun Zig codebase, treat `bun.hash()` as a one-shot helper: it hashes a single `[]const u8` buffer and is appropriate only for single-buffer, non-incremental hashing. For incremental hashing across multiple chunks (e.g., building a hash in a loop over header name/value pairs), use the wyhash streaming API directly: `std.hash.Wyhash.init(0)`, then call `.update(chunk)` for each piece, and finally `.final()` to produce the result. Do not apply the CLAUDE.md “use `bun.hash()` for 64-bit wyhash” guideline to incremental/multi-buffer cases—use `Wyhash.init/update/final` instead even though both ultimately rely on wyhash.
Learnt from: robobun
Repo: oven-sh/bun PR: 29137
File: src/bun.js/node/node_util_binding.zig:145-164
Timestamp: 2026-04-11T03:00:34.188Z
Learning: In this repo’s Zig code (oven-sh/bun), it’s acceptable to call `std.posix.isatty(fd_int)` directly when you already have a file descriptor as a raw `i32`. Do not treat direct `std.posix.isatty(fd_int)` calls as inconsistent just because `bun.sys.File.isTty()` exists—`bun.sys.File.isTty()` is essentially `std.posix.isatty(self.handle.cast())` and would require constructing a `bun.sys.File` first. Review should not flag direct `std.posix.isatty(fd_int)` usage as a `bun.sys` pattern violation.
Learnt from: robobun
Repo: oven-sh/bun PR: 29154
File: src/interchange/xml.zig:443-445
Timestamp: 2026-04-13T17:55:27.597Z
Learning: When reviewing Zig code in oven-sh/bun, note that `bun.strings.eqlLong(a, b, comptime check_len: bool)` performs a case-sensitive equality check. The `check_len` comptime bool only controls whether a length pre-check happens before the SIMD byte comparison, and is not related to case sensitivity. Therefore, do not treat `eqlLong(a, b, true)` (or any `check_len` value) as a case-insensitive comparison; only flag for case-insensitivity if the code actually uses a case-folding/comparison API intended for that purpose.
Learnt from: robobun
Repo: oven-sh/bun PR: 29422
File: src/bun.js/api/JSBundler.zig:0-0
Timestamp: 2026-04-18T04:44:25.893Z
Learning: In oven-sh/bun Zig code, follow the established OutOfMemory handling pattern: when you have an exhaustive `switch (err)` over the error union (enumerating every error variant), keep using `error.OutOfMemory => bun.outOfMemory()` inside the exhaustive switch arm. Do NOT recommend replacing `bun.outOfMemory()` with `bun.handleOom()` within those exhaustive switch arms. Only flag bare `catch bun.outOfMemory()` calls (i.e., non-exhaustive/non-switch handling), since `bun.handleOom()` is intended to wrap whole error-union expressions with `catch` to avoid accidentally swallowing non-OOM errors.
Learnt from: robobun
Repo: oven-sh/bun PR: 30147
File: src/bun.js/webcore/Body.zig:1777-1804
Timestamp: 2026-05-03T06:53:51.543Z
Learning: In this repo’s Zig code, avoid tail-calling (or otherwise delegating) between two mutually recursive functions when both rely on inferred error sets. Zig can fail with errors like “unable to resolve inferred error set” for the recursive cycle. If such mutually recursive structure is needed, apply the workaround used in Body.zig: inline the terminal-state handling directly (e.g., via an exhaustive switch) at the call site rather than tail-calling the other function that participates in the mutual recursion.
Learnt from: robobun
Repo: oven-sh/bun PR: 30219
File: src/resolver/package_json.zig:1731-1732
Timestamp: 2026-05-04T03:21:09.821Z
Learning: When reviewing this repo’s Zig code, avoid flagging repeated calls to `module_bufs.get()` (or other `bun.ThreadlocalBuffers(T).get()` instances) as redundant if they occur within the same function/scope. In `oven-sh/bun`’s `bun.ThreadlocalBuffers(T)` implementation, `get()` is `inline`, so after the first call the compiler typically reuses the same TLS value/register within that scope. For readability, it’s acceptable (and preferred) to call `get()` multiple times in the same scope rather than caching the value in a local variable and reusing it.
Learnt from: robobun
Repo: oven-sh/bun PR: 30314
File: src/cli/Arguments.zig:1046-1056
Timestamp: 2026-05-06T11:21:18.669Z
Learning: Boolean env var handling in Zig projects: In oven-sh/bun, env vars declared with kind.boolean (for example New(kind.boolean, \"NODE_USE_SYSTEM_CA\", .{ .default = false })) return a plain bool from .get(). The internal stringIsTruthy conversion is applied before returning the value, so callers should receive a bool, not an optional string. In code reviews, do not suggest capturing the result as ?[]const u8 (e.g., using |value|) or performing raw string comparisons like == \"0\" to infer truth for boolean-kind env vars. Rely on the .get() result and its boolean semantics. This guideline applies to files that read boolean env vars (e.g., src/cli/Arguments.zig) and related env_var usage (src/bun_core/env_var.zig).
Learnt from: robobun
Repo: oven-sh/bun PR: 30357
File: src/jsc/JSGlobalObject.zig:335-343
Timestamp: 2026-05-07T07:17:05.327Z
Learning: In this repo’s Zig code (e.g., Bun’s usage of `std.Io.Writer`), `std.Io.Writer.Error` is a single-variant set containing only `error{WriteFailed}`. Allocation/OOM failures are internally mapped to `error.WriteFailed` by `std.Io.Writer.Allocating.drain` (vendored `vendor/zig/lib/std/Io/Writer.zig`), so `error.OutOfMemory` is not catchable/distinguishable via `std.Io.Writer` error handling. Therefore, when reviewing `std.Io.Writer` operations, do not recommend splitting `catch` logic to handle `error.OutOfMemory` separately; a bare `catch` or `catch |err|` that only covers `error.WriteFailed` is correct.
Learnt from: ig-ant
Repo: oven-sh/bun PR: 30403
File: src/aio/MemoryPressureWatcher.zig:367-373
Timestamp: 2026-05-08T16:28:02.004Z
Learning: When reviewing Zig code, do not treat “accumulator-style” holdoff/elapsed-time loops as potentially wrong due to EINTR if the code measures elapsed time using repeated slices of `std.Thread.sleep` (e.g., `slept += N; std.Thread.sleep(N)`). On POSIX, `std.Thread.sleep` handles EINTR by restarting `nanosleep` with the remaining time (`req = rem; continue`), so each sleep slice consumes its full intended duration even if signals are delivered. In this case, an `slept += N` accumulator correctly tracks elapsed time without needing a deadline/`clock`-based approach. Only flag under-counting concerns if the code uses a sleep primitive that does NOT perform this EINTR retry behavior.
Learnt from: mchv
Repo: oven-sh/bun PR: 30625
File: src/test_runner/ScopeFunctions.zig:162-167
Timestamp: 2026-05-13T16:37:25.279Z
Learning: When calling `jsc.JSObject.createStructure(globalThis, owner, ...)` in Bun’s Zig bindings, ensure the `owner` argument is the JSValue that will actually hold references to the objects created from that structure (because it’s used only for the GC write barrier via `vm.writeBarrier(owner, structure)`). Do not pass a static context like `globalThis` or `vm` as the owner. Instead, pass a stable container such as `this.js_value` when callers cache the structure on a statement object, or pass a transient container like the result array JSValue when the structure is only needed for that array’s lifetime.
Learnt from: mchv
Repo: oven-sh/bun PR: 30625
File: src/test_runner/ScopeFunctions.zig:162-167
Timestamp: 2026-05-13T16:37:25.279Z
Learning: In this repo’s Zig code (oven-sh/bun), treat `jsc.JSObject.createStructure` as infallible: it internally performs GC-triggering heap allocations (`emptyObjectStructureForPrototype` + `addPropertyTransition`) but does not return a failure value (never returns null/invalid). During code review, do not flag the absence of validity/error checking on the returned `JSValue` from `createStructure` as a bug. Only apply this exemption to `jsc.JSObject.createStructure` (not other APIs).
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — minimal, well-scoped crash fix with regression tests.
Extended reasoning...
Overview
This PR fixes a fuzzer-discovered segfault in Bun.inspect() / console.log() and the jest pretty-formatter when handling malformed React elements. It touches three files: src/jsc/ConsoleObject.zig and src/test_runner/pretty_format.zig (mirrored fixes), plus four new regression tests in test/js/bun/util/inspect.test.js. The two code changes are (1) adding .JSX to canHaveCircularReferences() so the existing visited-set / stack-check guard applies to React elements, and (2) replacing the unsafe props.getObject().? unwrap with an if (props.getObject()) |props_obj| guard so primitive props values are skipped instead of panicking.
Security risks
None. This is purely defensive hardening of debug-output formatting code. The .JSX tag is only assigned when the value is already a JS object (js_type.isObject()), so adding it to the circular-reference set is safe and consistent with the other object-like tags already listed. The getObject() guard strictly narrows behavior (no new code path executes — it just skips the props block when props isn't an object) and removes a SAFETY: comment whose assumption was demonstrably false.
Level of scrutiny
Low. This is a small, mechanical crash fix in inspection/pretty-printing — non-semantic, output-only code that previously crashed on adversarial input. Both files receive the identical, idiomatic change (if |a| if |b| { ... };), and the surrounding control flow (defer, early return for children, trailing writer.writeAll(" />")) is preserved exactly. The canHaveCircularReferences mechanism is well-established for .Array/.Object/.Map/.Set and this just extends it to one more tag.
Other factors
Four targeted regression tests cover circular props, circular key, circular children, and five primitive props types. The PR description notes existing JSX inspect tests, console tests, and md-react.test.ts continue to pass. The bug-hunting system found no issues. No design decisions or behavioral trade-offs are involved — this is a straightforward Fuzzilli-found crash fix.
Collaborator
Author
|
Duplicate of #29709, which has the same fix with more comprehensive test coverage. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a segfault in
Bun.inspect()/console.log()(and the jest pretty formatter) when formatting React elements that have been mutated to contain circular references or non-objectprops.Root cause
.JSXtag was not included incanHaveCircularReferences(), so the visited-set / stack check that normally short-circuits cycles was skipped for React elements. A React element whoseprops,key, orchildrenreferred back to itself would recurse until the native stack overflowed.propsis always an object and didprops.getObject().?, which panics whenpropsis a primitive.Fix
.JSXtocanHaveCircularReferences()in bothConsoleObject.zigandpretty_format.zigso cycles print[Circular]instead of overflowing the stack.props.getObject()so non-object props are simply skipped.Repro
How did you verify your code works?
Added regression tests to
test/js/bun/util/inspect.test.jscovering circularprops, circularkey, circularchildren, and primitiveprops. Existing JSX inspect tests, console tests, andmd-react.test.tscontinue to pass.Found by Fuzzilli (fingerprint
13aabad674c61341).