Guard AggregateError .errors printing against self-reference - #35820
Guard AggregateError .errors printing against self-reference#35820robobun wants to merge 5 commits into
Conversation
… nesting A self-referential AggregateError (ae.errors contains ae) segfaulted every error-printing path (console.log, Bun.inspect, uncaught throw, unhandled rejection): the is_aggregate_error branch of print_errorlike_object iterates .errors via for_each -> agg_iter -> print_errorlike_object with neither a cycle guard nor a stack check, so a depth-1 cycle recurses until the native stack overflows. Add the same two protections the cause chain already has: the formatter's visited map (prints [Circular] on re-entry) and the stack_check guard (for deep non-cyclic nesting). Also seat StackCheck::init() in the two print_exception entry points so the guard actually fires on uncaught throw / unhandled rejection.
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 11:05 PM PT - Jul 25th, 2026
❌ @robobun, your commit a3ac642 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35820That installs a local version of the PR into your bun-35820 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Correct, this is a superset of #34892. #34892 adds only the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
These exercise the StackCheck::init() seating at the two print_exception entry points; the self-referential subprocess tests stop at depth 1 via the cycle guard and never reach the stack check.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
On the uncaught throw / unhandled rejection paths can_throw_stack_overflow is false, so when the stack guard trips no JS exception is thrown and for_each proceeds to the next .errors sibling. With fan-out > 1 this re-descends per sibling and turns O(D) into O(2^D). Match the ConsoleObject for_each callbacks and bail out once failed is set. The deep-nesting subprocess tests now use [e, e] to cover this.
There was a problem hiding this comment.
LGTM — both earlier review points are addressed (deep-chain throw/reject tests in 13c492a, formatter.failed short-circuit + fan-out=2 coverage in a3ac642).
Re-checked on this revision:
- The new guard block mirrors the existing
cause-chain guard at VirtualMachine.rs:6134-6164 (same pool-node init,get_or_put/found_existing, post-iterationremove). - All four
print_errorlike_objectentry points now havestack_checkseated: the two new sites here plus ConsoleObject.rs:727/1439/1502/5974 for theprint_error→print_errorlike_objectpath. agg_itershort-circuits onformatter.failedbefore recursing, matching the ConsoleObjectfor_eachcallbacks; the fan-out=2 subprocess test pins this.
Extended reasoning...
Overview
Crash fix in the error-printing path: a self-referential (or deeply nested) AggregateError previously SIGSEGV'd print_errorlike_object via unbounded native recursion through agg_iter. The change adds the same visited-map cycle guard and StackCheck guard that the cause-chain already has (~200 lines below in the same file), seats StackCheck::init() at the two print_exception entry points so the guard is live on the uncaught-throw / unhandled-rejection path, and short-circuits agg_iter on formatter.failed. Tests cover Bun.inspect, console.log, throw, and Promise.reject for both the self-referential and 16K-deep fan-out=2 shapes.
Prior review follow-up
I reviewed this PR twice before. The first pass flagged that the two new StackCheck::init() seatings had no test coverage (self-referential subprocess tests are caught by the cycle guard at depth 1; the deep-nesting test used Bun.inspect which seats stack_check independently). Fixed in 13c492a with a second it.concurrent.each for throw/Promise.reject over a deep non-cyclic chain. The second pass flagged that without a formatter.failed short-circuit in agg_iter, a fan-out ≥2 chain on the throw path re-descends per sibling for 2^D work — a hang. Fixed in a3ac642 with the short-circuit and by switching the deep-nesting test to new AggregateError([e, e]) so it hangs without the fix. Both threads are resolved.
Security risks
None. This is diagnostic-output code; the change turns a native stack overflow (SIGSEGV) into either a [Circular] marker or a controlled RangeError/silent truncation. No new user-controlled parsing, no allocation driven by untrusted sizes beyond what already existed.
Level of scrutiny
Moderate. print_errorlike_object runs on every uncaught exception, but the added block is a straight copy of the pattern already used for the cause chain in the same file (identical pool-node lazy-init, identical get_or_put/remove bracketing, identical is_safe_to_recurse → failed = true → optional throw_stack_overflow). I verified the fourth caller at ConsoleObject.rs:3997 passes an already-seated formatter (all its entry points seat stack_check at 727/1439/1502/5974), and that its temporary map.remove/scopeguard-reinsert dance composes correctly with the new get_or_put/remove here. The map_node lifecycle is unchanged — released via Formatter's Drop, same as before.
Other factors
Mechgate confirms every new test SIGSEGVs the runner on the unfixed build (release and ASAN) and passes with the fix. Every load-bearing clause — cycle guard, stack check, both StackCheck::init() seatings, and the agg_iter short-circuit — now has a test that fails without it. The comment-cop nit was addressed (comment trimmed to one line). Overlap with #34892 is acknowledged in the description as a superset with a trivial rebase either way.
There was a problem hiding this comment.
LGTM — both earlier findings (missing throw/reject deep-chain coverage, and the fan-out≥2 hang from not short-circuiting on formatter.failed) are addressed in 13c492a and a3ac642.
What was reviewed:
- New guard block mirrors
print_as_prelude(ConsoleObject.rs:3345-3383) and the cause-chain guard (VirtualMachine.rs:6139-6164) — same lazymap_nodeinit, same.expect("unreachable")onget_or_put, same post-iterationmap.remove. agg_iter'sformatter.failedshort-circuit makes unwind O(D) on the throw/reject path where no JS exception stopsfor_each; verified the fan-out=2 subprocess tests now cover this.StackCheck::init()seatings at bothprint_exceptionentry points are covered by the new deep-chainthrow/Promise.rejectsubprocess rows.
Extended reasoning...
Overview
Fixes a SIGSEGV when printing a self-referential or deeply-nested AggregateError on any of the four error-printing paths (console.log, Bun.inspect, uncaught throw, unhandled Promise.reject). Touches src/jsc/VirtualMachine.rs (+37), src/runtime/jsc_hooks.rs (+1), and test/js/node/util/bun-inspect.test.ts (+55). The fix adds a stack-check + visited-map cycle guard to the is_aggregate_error branch of print_errorlike_object, seats StackCheck::init() at the two print_exception entry points, and short-circuits the agg_iter for_each callback on formatter.failed.
Security risks
None material. This is defensive hardening of an error-printing path against pathological user-constructed inputs. The only user-reachable effect is that a value which previously crashed the process now prints [Circular] or truncates. No new allocation sizes are derived from untrusted lengths; the visited-map node comes from the existing formatter pool via the same code path already used by print_as_prelude and the cause-chain guard.
Level of scrutiny
Medium. It's native Rust in the JSC error-printing hot path, but the change is a near-verbatim copy of two existing in-file guard patterns (ConsoleObject.rs:3345-3383 and VirtualMachine.rs:6139-6164). I compared the new block line-by-line against both: same lazy map_node acquisition, same get_or_put/found_existing cycle check, same [Circular] output via pretty_fmt!, same post-recursion map.remove(&value) so sibling duplicates still print in full. The formatter.failed short-circuit in agg_iter matches the ConsoleObject for_each callbacks (2907/3003/3191). NonNull is already in scope (used at 6140).
Other factors
This PR has already been through two review iterations from me. The first (missing deep-chain coverage for the throw/reject StackCheck::init() seatings) was fixed in 13c492a. The second (2^D hang on fan-out≥2 because agg_iter didn't check formatter.failed) was confirmed by the author and fixed in a3ac642, with the deep-chain subprocess tests upgraded to fan-out=2 so they hang without the short-circuit. All threads are resolved. The mechgate evidence in the description shows the tests SIGSEGV on unfixed builds (both ASAN debug and release) and pass with the fix. The subprocess tests use it.concurrent.each, drain both pipes via Promise.all, and assert signalCode === null per the hang-guard convention. The bug-hunting system found nothing this run.
The known overlap with #34892 (stack-check-only subset) and #35174 (header ordering) is acknowledged in the description as trivial rebases whichever lands second.
|
CI is red on infrastructure, not this diff. Build #81680: the Rust side compiled cleanly on every lane that got an agent (linux-aarch64, linux-aarch64-musl, linux-x64-android, freebsd-x64 all show The diff itself is green: all 20 tests in |
|
Closing in favor of #36602, which consolidates the open AggregateError printing PRs. ae.errors.push(ae) now prints the header once followed by '[errors]: [Circular]' on every sink, with tests for the self and mutual cycle cases. |
2 participants
A self-referential
AggregateError(its own.errorsarray contains itself) SIGSEGVs every error-printing path:console.log,Bun.inspect, uncaughtthrow, and unhandledPromise.reject. Node prints[errors]: [ [Circular *1] ].Repro
3/3 on 1.4.0-canary and ASAN main; all four entries crash. No depth needed, a single self-reference is enough.
Cause
The
is_aggregate_errorbranch ofVirtualMachine::print_errorlike_objectiterates.errorsviafor_each->agg_iter->print_errorlike_objectwith neither a cycle guard nor a stack check. A cycle (or a sufficiently deep non-cyclic chain) recurses until the native stack overflows. Thecausechain already has both protections, inprint_error_instance_body/print_error_instance_js.Fix
src/jsc/VirtualMachine.rs: inside theis_aggregate_errorbranch, add the formatter's visited-map cycle guard (prints[Circular]on re-entry, removed after iteration so sibling duplicates still print in full) and astack_check.is_safe_to_recurse()guard. Same pattern as the existingcause-chain guard.src/jsc/VirtualMachine.rsandsrc/runtime/jsc_hooks.rs: seatformatter.stack_check = StackCheck::init()at the twoprint_exceptionentry points so the stack guard actually fires on uncaught throw / unhandled rejection (the defaultStackCheckhascached_stack_end == 0and never trips).Verification
New tests in
test/js/node/util/bun-inspect.test.tsalongside the existing deep-Error/object stack-overflow tests:Bun.inspectof a self-referential AggregateError returns a string containing[Circular]Bun.inspectof a 16K-deep AggregateError chain throwsRangeError: Maximum call stack size exceeded.console.log/Bun.inspect/throw/Promise.rejectof a self-referential AggregateError all print[Circular]withsignalCode === nullOn the unfixed build the first two segfault the test runner (exit 139) and all four subprocess variants exit 139.
Related
.errorsiteration to run after the aggregate header; orthogonal to this change, trivial rebase whichever lands second..errorsproperty) is a different failure mode.The bare
[Circular]output (no aggregate header) matches current Bun behaviour for AggregateError printing; #35174 addresses the header separately.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file