Seat StackCheck in both Formatter::new() constructors so deep non-Error values don't SIGSEGV the printer - #34884
Seat StackCheck in both Formatter::new() constructors so deep non-Error values don't SIGSEGV the printer#34884robobun wants to merge 6 commits into
Conversation
Throwing or rejecting a deeply nested non-Error value (e.g. a 50k-deep array) SIGSEGVed the process instead of printing the error and exiting 1. Formatter::new() leaves stack_check at StackCheck::default(), whose cached_stack_end is 0 so is_safe_to_recurse() always passes. format2 / Bun.inspect overwrite it with StackCheck::init(), but print_exception and the non-Exception branch of the jsc_hooks print_exception hook did not, so the print_as recursion guard never fired and print_array walked the value until the native stack overflowed. console.log of the same value correctly throws RangeError because its formatter is seated; the uncaught-exception printer is now at least as safe.
|
Reproduced with the repro in the PR body: stock bun and ASAN main both SIGSEGV (exit 139) at 50k depth; with the fix, 50k depth exits 1. Same crash in the test runner's Fix lives in both CI: the new tests in |
WalkthroughChangesFormatter recursion checks are initialized centrally, unsafe recursion now stops formatting, and regression tests cover deeply nested assertion, thrown-value, and rejected-value scenarios. Formatter recursion safety
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re the auto-linked #24069: that one is a different bug. Its stack trace shows the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Addresses review: the invariant belongs in the constructor. Formatter::new() takes a &JSGlobalObject so it is always on a JS thread where Bun__StackCheck__getMaxStack() returns the configured bound (and degrades to 0 = always-passes on an unconfigured thread, same as the old default). Drops the three per-call-site seatings added in the previous commit and the four pre-existing now-redundant ones in ConsoleObject.rs.
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/jsc/ConsoleObject.rs`:
- Around line 1729-1732: Prevent direct construction of Formatter from bypassing
StackCheck initialization by making Formatter and/or its fields private as
appropriate, or by exposing only constructor-based initialization. Ensure all
Formatter instances use Formatter::new and therefore initialize stack_check
through StackCheck::init, while preserving the existing formatting behavior.
In `@test/js/bun/util/reportError.test.ts`:
- Around line 147-149: Strengthen the assertions in the uncaught-error test
around the existing stderr and exitCode checks: separately verify that the
post-control uncaught-exception printer emits its expected marker or RangeError
output, rather than relying on the earlier console.log(a) catch output. Keep the
exit-code assertion and ensure the test fails if the uncaught printer exits with
code 1 without reporting the RangeError.
- Around line 126-128: Replace the regression comments near the
uncaught-exception printer and the corresponding lines around 145-146 with the
confirmed issue URL only, or remove them if no issue is tracked; do not retain
implementation history or behavioral explanation in those comments.
🪄 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: 4e2ecd41-bac0-4fa9-b29a-ebadc4ecc699
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/bun/util/reportError.test.ts
pretty_format::Formatter (the test runner's snapshot/diff formatter) is a separate struct from console_object::Formatter and had no recursion bound, so expect(50k-deep-array).toEqual([]) / .toMatchSnapshot() SIGSEGVed via the same mechanism. Add a stack_check field seated in new() and a is_safe_to_recurse() guard at the top of print_as, mirroring ConsoleObject. Test: use stderr: "ignore" and assert only signalCode/exitCode. On release builds the printer recurses ~20k-49k levels (Linux 8MB stack, 128KB guard) writing ~K^2 bytes before the guard fires; piping that into .text() crashed the Windows aarch64 lane in the previous CI run. Also drop the now-redundant stack_check.update() in JSValue::to_fmt (its only caller) and the stale "seated by the caller" comment in VirtualMachine.rs.
robobun
changed the title
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 `@test/js/bun/test/pretty-format-overflow.test.ts`:
- Around line 54-74: Strengthen both subprocess regressions by emitting a small
bounded-depth marker immediately before formatting or throwing, piping stdout or
another bounded stream so the marker remains observable, and asserting it while
continuing to ignore the potentially huge formatter diagnostics. In
test/js/bun/test/pretty-format-overflow.test.ts lines 54-74, verify the
50,000-level array was constructed before toEqual runs; in
test/js/bun/util/reportError.test.ts lines 126-141, verify each throw and
Promise.reject fixture reached the printer. Preserve the existing exit-status
and signal assertions.
🪄 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: 9e66d5fa-2f29-45dd-8345-deec8e3b1aec
📒 Files selected for processing (5)
src/jsc/JSValue.rssrc/jsc/VirtualMachine.rssrc/runtime/test_runner/pretty_format.rstest/js/bun/test/pretty-format-overflow.test.tstest/js/bun/util/reportError.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/JSValue.rs
|
The |
Tag::Proxy is not in can_have_circular_references(), so the early return at !can_circ skipped the is_safe_to_recurse() check and a 50k-deep Proxy chain still SIGSEGVed via print_proxy -> self.format recursion. Stack safety is orthogonal to circular-ref bookkeeping; move the check above the early return so it covers every recursive tag.
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 `@test/js/bun/util/reportError.test.ts`:
- Around line 126-130: Update the parameterized test title in the describe.each
block to include the stmt argument, so each case identifies whether it uses
throw or Promise.reject while preserving the existing wrap and type values.
🪄 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: c916b3aa-a140-4996-b486-d400c1a1732f
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/bun/util/reportError.test.ts
There was a problem hiding this comment.
All prior feedback from this thread has been addressed in 2a99f15 and 11ce721 — I don't have anything new to flag. Deferring to a human for sign-off given this changes the recursion-guard invariant for every Formatter::new() caller across two subsystems, and the pretty_format piece needs merge-order coordination with #34885.
Extended reasoning...
Overview
Seats a live StackCheck in both console_object::Formatter::new() and test_runner::pretty_format::Formatter::new(), reorders print_as_prelude so the stack guard runs before the !can_circ early-return (covering Tag::Proxy), removes the four now-redundant per-call-site stack_check = StackCheck::init() assignments plus the redundant .update() in JSValue::to_fmt, and adds subprocess regression tests for deep array/Proxy via throw/Promise.reject/toEqual.
Security risks
None. The change strictly narrows a native-stack-overflow crash surface (SIGSEGV → truncate + set failed). can_throw_stack_overflow stays defaulted to false, so newly-covered callers truncate rather than throw.
Level of scrutiny
Moderate-to-high. The mechanism is a one-line constructor swap plus a guard reorder, and StackCheck::init() is a cheap thread-local read that degrades to the old always-pass default on unconfigured threads. But it implicitly changes behavior for every Formatter::new() caller (~40 sites across 18 files), reorders a check in a hot recursive path so it now runs on every tag (including primitives), and touches two separate formatter structs. shallow_clone() was verified to propagate the seated stack_check (ConsoleObject.rs:1764).
Other factors
- All three prior review threads I raised (test output volume, stale comment/redundant
.update(),pretty_formatsibling, Proxy!can_circbypass) were addressed in follow-up commits and are resolved. - The verifier agents examined and refuted the concern that
stderr:"ignore"doesn't bound the child's own K² in-memoryDiffFormatterbuffer on release builds. - The PR description explicitly calls out overlap with #34885 (same
pretty_formatguard, broader test matrix) — whichever lands second needs a rebase, which is a maintainer coordination decision. - One trivial CodeRabbit nit (duplicate
describe.eachtitle string) was acknowledged as apply-at-merge; it doesn't affect test execution. - An earlier revision crashed the Windows aarch64 lane (build 76688) due to piped output volume; the current revision uses
stderr:"ignore"— worth confirming CI is green across all lanes on 11ce721 before merge.
There was a problem hiding this comment.
LGTM — all three prior rounds of feedback are addressed and the mechgate shows fail-before/pass-after on both ASAN and release.
What was reviewed:
StackCheck::init()seated in bothFormatter::new()constructors; verifiedshallow_clone()copies it and allto_fmtcallers pass freshly-constructed formatters, so the removed.update()was indeed dead.print_as_preludereorder: stack check now precedes the!can_circearly-return, so Proxy/JSX and other non-circular recursive tags are covered; the added per-leaf check is a pointer subtract + compare.- Tests use
stderr: "ignore"to avoid the K² release-build output, assertsignalCode === nullbeforeexitCode === 1, and thedescribe.eachtitle now distinguishes throw vs. Promise.reject.
Extended reasoning...
Overview
Fixes a native-stack-overflow SIGSEGV when the console/test-runner formatter recurses into a deeply nested non-Error value (50k-deep array, Proxy chain) via three coordinated changes: (1) console_object::Formatter::new() seats stack_check: StackCheck::init() instead of default() and the four now-redundant per-call-site re-seats plus the to_fmt .update() are deleted; (2) print_as_prelude checks is_safe_to_recurse() before the !can_circ early-return so Tag::Proxy and other non-circular recursive tags hit the guard; (3) test_runner::pretty_format::Formatter gains a stack_check field and an is_safe_to_recurse() guard at the top of print_as. A stale comment in VirtualMachine.rs is trimmed. Four new subprocess tests cover throw/reject × array and throw × Proxy for the uncaught printer, plus expect(50k-deep).toEqual([]) for the diff formatter.
Security risks
None introduced. This closes a user-reachable native crash (DoS-class) — throwing a deep value would SIGSEGV the process. StackCheck::init() reads a cached thread-local; on unconfigured threads it returns 0 (identical to the old default), so no behavior change off the JS thread. can_throw_stack_overflow stays defaulted to false, so the uncaught-exception printer and diff formatter truncate + set failed rather than throwing into a context that can't catch.
Level of scrutiny
Medium-high — this is a hot-path native formatter used by every console.log, uncaught-exception print, and test-runner diff. The change itself is small and mechanical (a constructor default swap, a 6-line reorder, a mirrored field+guard in the sibling formatter). I reviewed this PR three times previously; each finding (missing pretty_format sibling, stale comment/dead .update(), K² stderr in the test, Proxy !can_circ bypass) was addressed in a follow-up commit. This run I re-verified: the reordered print_as_prelude at ConsoleObject.rs:3384-3393 is correct; shallow_clone() at :1764 copies stack_check from self; pretty_format::print_as at :1161-1164 has the guard; and a spot-check of to_fmt callers confirms they all pass freshly-constructed formatters, so removing the .update() cannot leave a stale bound.
Other factors
The PR body's mechgate evidence shows the new tests SIGSEGV on main (ASAN: 5 fail; release: Proxy variant fails) and pass with the fix on both profiles. CI build 76704 passed with three unrelated flakes (darwin websocket idle-CPU, two install-test flakes). All CodeRabbit findings are resolved/withdrawn. The overlap with #34885 is noted in the PR body — whichever lands second needs a trivial pretty_format.rs rebase. The bug-hunting system found nothing on this final revision.
1 participant
Throwing or rejecting a deeply nested non-Error value (e.g. a 50k-deep nested array or Proxy chain) SIGSEGVs the process instead of printing the error and exiting 1. The same value SIGSEGVs the test runner when rendered by the snapshot/diff formatter (
expect(a).toEqual([])).Repro
3/3 on 1.4.0 and ASAN main. Node prints a truncated
[ [ [ [Array] ] ] ]and exits 1.Cause
console_object::Formatter::new()leftstack_checkatStackCheck::default(), whosecached_stack_endis 0 sois_safe_to_recurse()always returns true.format2/Bun.inspectoverwrote it withStackCheck::init(), but ~40 other callers (includingVirtualMachine::print_exception) did not. Theprint_as_preluderecursion guard never fired for those, andprint_array(which has nomax_depthclamp of its own) walked the value until the native stack overflowed.print_as_preludealso returned early at!can_circbefore the stack check, soTag::Proxy(not incan_have_circular_references()) bypassed the guard entirely viaprint_proxy->self.formateven wherestack_checkwas seated.test_runner::pretty_format::Formatteris a separate struct (used bytoEqual/toMatchSnapshotdiff rendering) with nostack_checkfield and no depth bound at all; itsprint_asrecursed the same way.Fix
console_object::Formatter::new()seatsstack_check = StackCheck::init()itself. It is a single read of a cached thread-local, and every caller is already on a JS thread. On an unconfigured thread the FFI returns 0, same as the old default. The pre-existing explicit seatings in ConsoleObject.rs and the redundant.update()inJSValue::to_fmtare removed.print_as_preludechecksis_safe_to_recurse()before the!can_circearly-return so the stack guard covers every recursive tag (array, Proxy, JSX, ...).pretty_format::Formattergains astack_checkfield seated innew()and anis_safe_to_recurse()guard at the top ofprint_as.can_throw_stack_overflowstays defaulted tofalse, so callers that don't opt in (the uncaught-exception printer, the diff formatter) truncate and setfailedrather than throwing.Verification
New tests in
test/js/bun/util/reportError.test.tsspawn the 50k-deep repro viathrow/Promise.reject(array) andthrow(Proxy chain) and assertsignalCode === null/exitCode === 1. New test intest/js/bun/test/pretty-format-overflow.test.tsspawns abun testfixture that doesexpect(a).toEqual([]). All four SIGSEGV on the unfixed build and pass with the fix.The tests use
stderr: "ignore": on release builds the printer recurses ~20k-49k levels (8MB Linux stack, 128KB guard) writing ~K^2 bytes on the way down, so piping stderr would make the test itself the bottleneck.Existing coverage (
bun-inspect.test.ts16k-deep Error chain,inspect.test.js,snapshot-tests/,expect.test.js,console/) all pass.The
pretty_formatpiece overlaps with #34885 (same guard, broader test matrix there); whichever lands second needs a trivial rebase.[review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file