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.
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