Guard print_errorlike_object against unbounded AggregateError recursion - #34892
Guard print_errorlike_object against unbounded AggregateError recursion#34892robobun wants to merge 3 commits into
Conversation
Printing a deeply nested AggregateError (e.g. 2k levels of new AggregateError([inner])) SIGSEGVs every error-print path: console.log, Bun.inspect, uncaught throw, and unhandled rejection. The AggregateError branch of print_errorlike_object iterates .errors via JSValue::for_each -> agg_iter -> print_errorlike_object with no stack check, so the recursion runs until the native stack overflows. The Error cause chain is guarded one level deeper in print_error_instance_js, but that guard is only reached once the AggregateError loop bottoms out. Additionally, VirtualMachine::print_exception and the jsc_hooks print_exception entry point construct a Formatter with the default (unseated) StackCheck, so the existing cause-chain guard never fired on the uncaught-throw / unhandled-reject paths either. Fix: hoist the formatter's is_safe_to_recurse() check to the top of print_errorlike_object (matching the pattern in print_error_instance_js), and seat formatter.stack_check at the two print_exception entry points.
|
Updated 1:18 AM PT - Jul 21st, 2026
✅ @robobun, your commit 383f3bc90498396489248b00e5d2b021f45c6e5f passed in 🧪 To try this PR locally: bunx bun-pr 34892That installs a local version of the PR into your bun-34892 --bun |
|
Reproduced: 383f3bc flattens the subprocess matrix to Re the auto-suggested issues: #1352, #21528 and #32343 are all about how AggregateError / |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe change initializes stack tracking during exception formatting, prevents unsafe recursive traversal of AggregateError stack safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/js/node/util/bun-inspect.test.ts`:
- Around line 100-101: Replace the sequential describe.each matrix around
“printing a deeply nested error via %s” with a flat test.concurrent.each matrix,
preserving the four ["log", "throw", "reject", "cause"] cases and the existing
test body so all subprocess cases run concurrently.
- Around line 117-119: Update the test around the Promise.all calls to retain
the formatter’s stdout and stderr instead of discarding them, then assert the
expected stable per-case error or truncation marker for each case in addition to
proc.signalCode and exitCode. Keep the existing termination assertions while
ensuring an unrelated exit code 1 cannot satisfy the test.
🪄 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: 5d07f236-5aef-4490-ae9e-1688eee44408
📒 Files selected for processing (3)
src/jsc/VirtualMachine.rssrc/runtime/jsc_hooks.rstest/js/node/util/bun-inspect.test.ts
Switch to it.concurrent.each (flatter, same concurrency) and write a 'built' marker to stderr after constructing the 16K-deep chain so the test asserts the child actually reached the print/throw rather than passing on an unrelated exit-1.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused crash fix that mirrors the existing print_error_instance_js stack guard.
What was reviewed:
- Verified the new guard in
print_errorlike_objectis byte-identical in shape to the existing one at VirtualMachine.rs:5625-5634 (failed = true+ conditionalthrow_stack_overflow), and that a defaultStackCheck(cached_stack_end == 0) makesis_safe_to_recurse()vacuously true, so callers that don't seat it are unaffected. - Confirmed all four
print_errorlike_objectcall sites now reach it with a seatedstack_check: the two patchedprint_exceptionentry points, the recursiveagg_iter, and ConsoleObject.rs:4036 (whose formatter is seated byformat2/Bun.inspectat ConsoleObject.rs:727/1439/1502/6018). - Subprocess tests:
stdout: "ignore"avoids pipe backpressure on theconsole.logcase, stderr is drained, and thebuilt\nmarker +signalCode === nullguard against unrelated exit-1.
Extended reasoning...
Overview
Three-file change fixing a native stack overflow (SIGSEGV) when printing deeply nested AggregateError chains. src/jsc/VirtualMachine.rs gains an is_safe_to_recurse() guard at the top of print_errorlike_object (7 lines) plus a one-line StackCheck::init() seating in VirtualMachine::print_exception. src/runtime/jsc_hooks.rs gets the same one-line seating in its print_exception else-branch. test/js/node/util/bun-inspect.test.ts adds one in-process Bun.inspect test alongside the existing 16K-deep object/Error tests, plus a 4-case it.concurrent.each subprocess matrix (log/throw/reject on AggregateError + throw on cause-chain).
Security risks
None. This is the error-printing path; the change converts a user-triggerable SIGSEGV into a clean RangeError or silent truncation, which is strictly a DoS-hardening improvement. No new user input is parsed and no allocations are added.
Level of scrutiny
Low-to-medium. The native change is tiny and copies an established pattern verbatim from the same file (print_error_instance_js at line ~5625). The StackCheck::init() seating matches the existing idiom used four times in ConsoleObject.rs. The PR description notes #34884 will move the seating into Formatter::new() itself; whichever lands second has a trivial two-line rebase. The default-StackCheck semantics (cached_stack_end == 0 → guard never trips) mean the new check is inert for any call path that doesn't seat it, so there's no risk of regressing shallow error printing.
Other factors
The whole-class check holds: I grepped all four callers of print_errorlike_object and each now reaches it with a seated stack_check. The many other Formatter::new() sites (test_runner/expect/validators) don't route through print_errorlike_object and are out of scope here (covered by #34884). Test coverage is solid: the in-process test asserts the exact RangeError message via inline snapshot (matching the two neighboring tests), and the subprocess matrix asserts a post-construction built\n marker on stderr before checking signalCode === null and exitCode === 1, so an unrelated early failure can't satisfy it. Both CodeRabbit review comments were addressed in 383f3bc and marked resolved. No outstanding human review comments.
|
Closing in favor of #36602, which consolidates the open AggregateError printing PRs. The .errors members are now printed through the same path as the cause chain, whose stack check is seated for the uncaught and unhandled-rejection reporters there as well, and a 3000-deep chain is covered by tests on console.error (RangeError), uncaught throw and unhandled rejection. |
1 participant
Printing a deeply nested
AggregateErrorSIGSEGVs every error-print path:console.log,Bun.inspect, an uncaughtthrow, and an unhandledPromise.reject. Node prints the chain and exits 1.Repro
3/3 on 1.4.0-canary and ASAN main; ~1000 levels prints fine, ~2000 crashes. The equivalent
causechain at the same depth throws a cleanRangeErrorfromBun.inspect.Cause
The AggregateError branch of
VirtualMachine::print_errorlike_objectiterates.errorsviaJSValue::for_each->agg_iter->print_errorlike_objectwith no stack check. The recursion stays inside that branch (each inner error is also an AggregateError) and never reaches the guardedprint_error_instance_jspath until the leaf, so the native stack overflows first.Separately,
VirtualMachine::print_exceptionand thejsc_hooks::print_exceptionentry point both construct a freshFormatterwith the defaultStackCheck(cached_stack_end == 0, always reports safe), so the existingcause-chain guard inprint_error_instance_jsnever fires on the uncaught-throw / unhandled-reject paths either. Throwing a 5000-deepcausechain also SIGSEGVs today.Fix
src/jsc/VirtualMachine.rs: hoist the formatter'sis_safe_to_recurse()check to the top ofprint_errorlike_object, matching the existing pattern inprint_error_instance_js. When the limit is hit the formatter throwsRangeError(forBun.inspect/console.log, which opt in viacan_throw_stack_overflow) or truncates silently (uncaught printer), same as thecausechain does today.src/jsc/VirtualMachine.rs,src/runtime/jsc_hooks.rs: seatformatter.stack_check = StackCheck::init()at the twoprint_exceptionentry points so the guard actually fires on the throw/reject paths.Verification
New tests in
test/js/node/util/bun-inspect.test.tsalongside the existing 16K-deep Error/object stack-overflow tests:Bun.inspectof a 16K-deep AggregateError throwsRangeError: Maximum call stack size exceeded., and spawned subprocesses coveringconsole.log/throw/Promise.rejectof a 16K-deep AggregateError plusthrowof a 16K-deepcausechain all exit 1 with no signal.On the unfixed build the in-process test SIGSEGVs the runner and all four subprocess variants exit 139; with the fix all 17 tests in the file pass.
Related: #34884 seats
StackCheckinsideFormatter::new()for the deep non-Error (array/Proxy) throw path; that change does not on its own cover the AggregateError loop here, which never reaches a checked function. Whichever lands second has a trivial rebase of the two seating lines. Also distinct from #31988 (tampered.errorsproperty).[stamp-90s] 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