console: guard AggregateError .errors recursion (cycle, depth, tampered property) - #35825
console: guard AggregateError .errors recursion (cycle, depth, tampered property)#35825robobun wants to merge 11 commits into
Conversation
…ck check, and property validation The AggregateError branch of print_errorlike_object walked .errors via for_each -> agg_iter -> print_errorlike_object with no visited set, no stack_check, and no validation of the getDirect result. A self/mutual cycle, a 3000-deep chain of aggregates, or a deleted/accessor .errors property all reached the stack guard page (silent SIGSEGV) on every sink that uses the native error printer (console.log/error, Bun.inspect, uncaught throw, unhandled rejection, uncaughtException handler). The cause-chain printer already has both guards (stack_check at print_error_instance_js and the formatter.map [Circular] check), so the fix reuses the same visited-pool idiom keyed on the AggregateError object identity, adds the stack_check at the top of the branch, and only iterates when .errors is an object (clearing any exception for_each may raise). Seating StackCheck::init() on the two run_error_handler formatter sites makes the existing cause-chain depth guard effective on the uncaught-throw / unhandled-reject sinks, where Formatter::new left it defaulted and therefore always-true. With the guards in place the branch now falls through to print the AggregateError's own name/message/stack before its children, so an uncaught AggregateError shows its own message instead of only its members.
|
Warning Review limit reached
Next review available in: 13 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 (4)
Comment |
|
Status: diff is green.
Locally 47/47 pass on debug+ASAN (~7s) and release (~0.4s); 32/47 segfault on an unfixed build. Verified under |
|
Updated 10:15 PM PT - Jul 25th, 2026
❌ @robobun, your commit 055856f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35825That installs a local version of the PR into your bun-35825 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…ndent on release)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this restructures control flow in the native error printer and changes user-visible output (AggregateError now prints its own header before children on uncaught throw — the #21528 fix), so it's worth a human look. There are also 7 open PRs listed as duplicates on the same code path; a maintainer should pick which lands and close the rest.
What was reviewed:
- Visited-map /
get_or_put/removeidiom matches the existing cause-chain guard (VirtualMachine.rs:6141) andFormattercycle check (ConsoleObject.rs:3363). StackCheck::init()seating matches the pattern at ConsoleObject.rs:727/1439/1502/5974;Formatter::newleaves it asdefault()(always-pass), so seating it here is required.get_errors_propertyisgetDirect(nothrow);is_object()gates out empty and GetterSetter;for_eacherrors are cleared.- Ruled out double-printing of
.errors(property-dump path vs.for_eachiteration are on separate branches).
Extended reasoning...
Overview
The PR guards the AggregateError branch of print_errorlike_object in src/jsc/VirtualMachine.rs with a stack-depth check, a visited-set cycle check (reusing formatter.map), and validation of the .errors property before iterating it. It also moves the print_error_from_maybe_private_data call above the children loop so an uncaught AggregateError now prints its own name/message/stack (fixing #21528). Two Formatter::new sites (VirtualMachine::print_exception, jsc_hooks::print_exception) now seat StackCheck::init() so the pre-existing cause-chain depth guard actually fires on the uncaught/reject sinks. A new 48-cell subprocess test matrix covers {self-cycle, mutual-cycle, deleted/accessor/non-iterable .errors, mixed agg+cause, deep-aggregate, deep-cause} × {console.log, console.error, Bun.inspect, uncaught-throw, unhandled-reject, uncaughtException-handler}.
Security risks
None. The change is defensive (prevents user-triggerable stack-overflow segfaults in the error printer). No new user input reaches native code that didn't before; get_errors_property remains getDirect (no getter invocation), and any exception raised inside for_each is now cleared rather than swallowed via let _ =.
Level of scrutiny
Moderate-to-high. The error printer runs on every uncaught exception, unhandled rejection, and console.error; a regression here degrades every crash report. The visited-map and stack-check additions are straight copies of existing idioms in the same file, which reduces risk. The larger judgment call is the output-format change: AggregateError uncaught output will now include an extra header line, which is the desired fix for #21528 but is a user-visible change that could touch downstream snapshot expectations. That, plus the 7 open competing PRs on the same lines (#34892, #35820, #35174, #31988, #35816, #35826, #35050), makes this a maintainer decision rather than a mechanical approve.
Other factors
My prior nit (serial describe on the depth block) was addressed in b7b1a39; the file now runs ~9s under debug+ASAN. The comment-cop bot flags were on pre-existing comments that moved in the diff (author confirmed) and are all resolved. The evidence block's "release without fix: all passed" label is confusing (the hash shown is a PR commit), but robobun separately confirmed 37/48 cells segfault under USE_SYSTEM_BUN=1, which is the load-bearing fails-on-main proof.
…g for_each with a pending exception Skip the .errors iteration when the header print already tripped the inner stack guard (formatter.failed or pending exception), and only clear a for_each error when formatter.failed is unset. Bun.inspect / console.* on a very deep AggregateError now throws RangeError like it does for a deep cause chain.
…ter.failed formatter.failed is sticky across siblings on can_throw_stack_overflow=false sinks, so a deep branch would suppress every following sibling's .errors expansion. has_exception() alone covers the can_throw=true RangeError case; the post-for_each !failed guard still prevents clearing a propagating RangeError.
…tually threw Narrow the post-for_each clear guard to !(failed && can_throw_stack_overflow) so a hostile .errors iterator after a deep sibling is still cleared on the uncaught-throw / unhandled-reject sinks, while the RangeError on the Bun.inspect / console.* sinks keeps propagating.
There was a problem hiding this comment.
Nothing further from me — the three earlier inline threads are all addressed at 055856f. Deferring to a human because this reorders the AggregateError branch to print the AggregateError: <msg> header on every uncaught/rejected AggregateError (fixing #21528 as a side effect of the crash guard), which is a user-visible output-format change; and because it supersedes ~7 open sibling PRs that a maintainer will want to close alongside it.
What was reviewed:
- Visited-map insert/remove in the aggregate branch mirrors the existing cause-chain idiom at L6142-6167, and both paths share
formatter.mapso themixed-agg-causecycle is caught. errors.is_object()correctly rejects theGetterSettercell thatgetDirectreturns for accessor.errors, so the throwing-getter shape never entersfor_each.- The final
!(formatter.failed && formatter.can_throw_stack_overflow)guard: oncan_throw=truesinks the RangeError propagates; oncan_throw=falsesinks a hostile-iterator sibling after a deep branch is now cleared. - Grepped for other snapshot tests asserting the old header-less AggregateError output; only
jsx-template-string-crash.test.tsneeded updating.
Extended reasoning...
Overview
The PR guards the AggregateError .errors recursion in VirtualMachine::print_errorlike_object against three segfault vectors — self/mutual cycles (via the existing formatter.map visited pool), deep chains (via formatter.stack_check.is_safe_to_recurse()), and tampered .errors (deleted / accessor / non-iterable, via is_object() gate + post-for_each clear_exception). It also seats StackCheck::init() on the two run_error_handler formatter sites so the pre-existing cause-chain depth guard is actually live on the uncaught-throw / unhandled-reject sinks. As a structural consequence of moving print_error_from_maybe_private_data above the .errors iteration, an uncaught AggregateError now prints its own AggregateError: <msg> header before its children — which is the feature request in #21528.
Files touched: src/jsc/VirtualMachine.rs (+67/-26), src/runtime/jsc_hooks.rs (+1), a new 48-cell subprocess test matrix, and a 2-line snapshot update in jsx-template-string-crash.test.ts for the new header.
Security risks
None identified. This is defensive hardening of the error printer against adversarial error-object shapes; the pre-PR behavior was a segfault (DoS) on all of these inputs, so every path is strictly safer. No new user-controlled data reaches an allocation size, path, or FFI boundary.
Level of scrutiny
High. print_errorlike_object sits on every uncaught-exception / unhandled-rejection / console.error / Bun.inspect path. The exception-propagation semantics around for_each are subtle enough that they took three review rounds here (swallowing an intentional RangeError → over-suppressing sibling iteration via sticky formatter.failed → leaking a hostile-iterator exception on non-throwing sinks), each addressed in a follow-up commit. The current shape at 055856f looks correct to me on all four sink families.
Other factors
- User-visible output change: every multi-error
bun -eparse failure and every uncaughtAggregateErrornow gains a leadingAggregateError: N errors …line. That is the requested behavior in #21528, but it's a format change a maintainer should sign off on rather than a bot. - Duplicate PRs: the find-duplicates bot lists seven open PRs (#34892, #35820, #35174, #31988, #35816, #35826, #35050) each covering a subset of this fix. A human should pick this one (or one of those) as canonical and close the rest.
- Test coverage is thorough (7 shapes × 6 sinks + depth + output assertions, all
describe.concurrent, ~9s under debug+ASAN per the author). The gate evidence shows 38 of the 48 cells crash on unfixed ASAN and all pass with the fix. - The comment-cop bot flagged five long comments in
VirtualMachine.rs; the author noted (and I confirmed) that they are pre-existing onmainand only moved in the diff.
|
This also fixes #36963, which hits the deleted/missing I verified a minimal guard at the same site fixes it, with regression tests for the two-test-file repro and the |
|
Closing in favor of #36602, where this has been folded in: the stack check is seated for the uncaught-exception and unhandled-rejection reporters (both now build their formatter in VirtualMachine::print_exception; Formatter::stack_check became pub(crate) in #36184, so the jsc_hooks.rs change here no longer compiles as written), the .errors walk only runs when the property is still an array, and the visited set covers self/mutual cycles and the mixed cause/errors cycle. The test matrix from this PR (cycle, deleted, accessor, non-array, deep aggregate and deep cause chains on the console and uncaught sinks) and a test for the #36963 bun test repro live in test/js/bun/util/inspect-error.test.js there. |
2 participants
Reproduction
All of the following segfault (silent 139, ASan prints nothing) on every sink that reaches the native error printer (
console.log/console.error,Bun.inspect, uncaught throw, unhandled rejection,uncaughtExceptionhandler, and the in-Worker uncaught path):And a very deep plain
causechain segfaults on the uncaught/reject sinks only:Cause
The AggregateError branch of
print_errorlike_objectwalks.errorsviafor_each->agg_iter->print_errorlike_objectwith no visited set, nostack_check, and no validation of thegetDirect(errors)result. At ~8 KB of native stack per level the recursion reaches the guard page. Thecausechain already has both guards inprint_error_instance_js, but therun_error_handlerformatter never seatedStackCheck::init(), so that depth guard was inert on the uncaught-throw / unhandled-reject sinks.Fix
In the aggregate branch:
stack_check.is_safe_to_recurse()at entry, writing[AggregateError: nesting too deep]and returning when it fails.formatter.mapvisited pool (sameget_or_put/found_existing->[Circular]/removeidiom the cause-chain uses) keyed on the AggregateError object, inserted before printing and removed after..errorsis an object; clear any exceptionfor_eachraises. This covers deleted, accessor, and non-iterable.errors.With the guards in place the branch now falls through to print the AggregateError's own name/message/stack before its children, so an uncaught
new AggregateError([inner], "outer")renders theAggregateError: outerheader (previously only the inner errors were printed).StackCheck::init()is seated on bothrun_error_handlerformatter sites (VirtualMachine::print_exceptionandjsc_hooks::print_exception), making the existing cause-chain depth guard effective on those two sinks.Verification
test/js/bun/util/inspect-error-cycle.test.tsspawns a child per{shape} x {sink}cell (7 shapes x 6 sinks, plus 6 deep-aggregate cells and 2 deep-cause cells, plus 4 output assertions) and asserts no signal. 37/48 cells crash on currentmain; all 48 pass with this change.Supersedes the partial fixes in #34892 (depth only), #35820 (self-ref only), #35174 (header only), and #31988 (tampered property only).
Fixes #21528
[review] gate passed · iteration 4 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 2 rejected · iteration 4
evidence per changed file