Error instead of crashing on deeply nested statements in the transpiler#31333
Conversation
Deeply nested blocks (~600-1000 levels of '{') parsed fine but overflowed
the stack in the visit pass: the visit_stmts -> visit_and_append_stmt ->
s_block cycle had no stack check, unlike the expression path. Add the same
is_safe_to_recurse() guard to visit_and_append_stmt and print_stmt so deep
statement nesting reports 'Maximum call stack size exceeded' instead of
dying with SIGSEGV.
|
Updated 9:07 PM PT - May 24th, 2026
❌ @robobun, your commit 66e1e33 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31333That installs a local version of the PR into your bun-31333 --bun |
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 53 minutes and 24 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward application of the existing expression-recursion stack guards (#31242) to the statement recursion path.
Extended reasoning...
Overview
This PR adds stack-depth guards to two recursive statement-processing functions (visit_and_append_stmt in the visitor and print_stmt in the printer), updates stale comments in p.rs/parse_stmt.rs that previously stated the visitor/printer were unguarded, and adds a subprocess-based regression test mirroring the existing deeply nested expressions test directly above it.
Security risks
None. This is a crash-hardening fix: it converts a fuzzer-found SIGSEGV (stack overflow on ~600 nested { blocks) into a catchable Maximum call stack size exceeded error. No new attack surface, no parsing/semantic changes for valid inputs.
Level of scrutiny
Low-to-medium. The change is mechanical: the new guard in visit_and_append_stmt is byte-for-byte the same pattern as visit_expr_in_out (visit_expr.rs:49-52), and the new guard in print_stmt is identical to those already in print_expr (lib.rs:3273-3276) and print_binding (lib.rs:4970-4973). The early return Ok(()) is safe because report_stack_overflow / stack_overflowed ensure an error surfaces to the caller, and the only effect on well-formed shallow code is one cheap stack-pointer check per statement.
Other factors
The bug-hunting system found no issues. The regression test is well-constructed (runs in a subprocess so a crash fails the test rather than the test runner, exercises three nesting shapes at three depths, checks both tsx+DCE and plain js loaders). The comment updates are accurate. CI build is in progress with no prior reviewer comments to address.
else-if chains recurse print_if -> print_if directly without passing through print_stmt, and t_if parses them iteratively so they are not bounded by MAX_STMT_DEPTH either. Give print_if the same stack check as print_stmt/print_expr, and cover else-if chains in the nesting test.
There was a problem hiding this comment.
LGTM — the print_if guard and else-if test shape address my earlier nit, and all four new guards mirror the existing visit_expr_in_out/print_expr pattern exactly.
Extended reasoning...
Overview
This PR adds stack-depth guards to the statement-recursion paths of the transpiler, mirroring the expression-recursion guards added in #31242. Four is_safe_to_recurse() checks are added (entry of visit_and_append_stmt, print_stmt, and print_if), plus comment-only updates to p.rs/parse_stmt.rs reflecting that the visitor/printer are no longer unguarded, and a subprocess-based regression test covering four nesting shapes at depths 600/800/990.
Security risks
None. The change is purely additive early-return guards on pathological-depth inputs; it converts a fuzzer-found SIGSEGV into a catchable error. No new attack surface, no parsing/semantic changes for normal inputs.
Level of scrutiny
Low-to-moderate. Each guard is a 4-line copy of the pattern already in visit_expr_in_out (visit_expr.rs:49-52), print_expr (lib.rs:3273-3276), and print_binding (lib.rs:4970-4973) — same stack_check.is_safe_to_recurse() predicate, same reported_stack_overflow/stack_overflowed flag, same early return. The only design question (whether print_if's direct self-recursion on else-if chains needed its own guard) was raised in my earlier review and addressed in ab83e14. Since is_safe_to_recurse() already fires on every visit_expr_in_out call, adding it to the (less frequent) per-statement entry introduces no new false-positive risk surface.
Other factors
- My earlier inline nit is resolved: the
print_ifguard was added and anelse ifchain shape was added to the test. - The new test follows the same subprocess-isolation pattern as the adjacent
deeply nested expressionstest, and the/Maximum call stack size exceeded|StackOverflow/regex matches both the visitor's reported message and the parse-timeerr!("StackOverflow"). - No bugs found by the bug-hunting system on the latest revision.
|
CI status (latest run, build 57731 on 66e1e33, and the two runs before it): every lane that actually executes tests is green — 73/73 checks passed, including Linux glibc/musl x64/aarch64 (release + ASAN), Windows, and macOS x64, all of which run The only red checks on each run are the macOS aarch64 test lanes ( |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Delete this max statement depth. It should never have been added. Use bun.StackCheck instead.
parse_stmt now guards recursion with stack_check.is_safe_to_recurse() alone, like the Zig parser. The hard cap existed to protect the visitor/printer while they were unguarded; they now carry their own stack checks.
|
Done in 63bedce — removed |
Without the parse-depth cap, the pre-visit scope-tree walk in hoist_symbols sees the full nesting depth the parser allowed; in release builds the small parse frames let ~15k-deep inputs through, and the unguarded recursion overflowed the stack.
…ted-stmt-stack-overflow # Conflicts: # test/bundler/transpiler/transpiler.test.js
* oven/main (20 new commits): webcore: free Blob's owned content type on drop (oven-sh#31358) Support cross-compiling macOS binaries from Linux (oven-sh#31303) test: forward keep-alive requests in proxy.test.ts's mock proxy (oven-sh#31352) Port Bun.stringWidth to C++ with explicit SIMD (oven-sh#31351) Fix quadratic hang reporting duplicate-binding parse errors in the transpiler (oven-sh#31341) shell: don't abort when a glob's directory prefix doesn't exist (oven-sh#31367) Error instead of crashing on deeply nested statements in the transpiler (oven-sh#31333) Fix JSX transform panic when a bare `key` prop precedes `key` with a value (oven-sh#31350) Cap ANSI markdown indentation so deeply nested lists render in linear time (oven-sh#31366) css: bound selector-list expansion when compiling nesting for older targets (oven-sh#31277) node:http2: reassemble HEADERS+CONTINUATION before HPACK decoding (oven-sh#31323) Fix `await using` expression printing `using` as `await` (oven-sh#31324) Parenthesize `async` when it starts a for-of loop initializer (oven-sh#31326) Print Infinity and negative numeric property keys as computed properties (oven-sh#31328) css: keep required grouping parens in @container conditions when minifying (oven-sh#31330) Fix panic on anonymous export default class with an auto-accessor field (oven-sh#31331) node:http2: send GOAWAY frames on stream 0 (oven-sh#31353) parser: fix Scope mismatch while visiting panic from decorators on dropped class members (oven-sh#31340) webcrypto: reject oversized BufferSource inputs instead of aborting (oven-sh#31356) Error instead of crashing on deeply nested TypeScript types in the transpiler (oven-sh#31361) Resolved conflicts: - scripts/build.ts: kept both OHOS and macOS-cross argv entries - scripts/build/config.ts: kept both OHOS and macOS-cross config fields - scripts/build/deps/webkit.ts: kept OHOS fno-pic exclusion, adopted upstream -flto=thin
…tests throw on Windows (#31382) ### Problem Every PR build since #31333 merged fails the Windows test lanes (2019 x64, 2019 x64-baseline, 11 aarch64) on the same two tests in `test/bundler/transpiler/transpiler.test.js`: ``` ✗ Bun.Transpiler.transformSync stack overflows ✗ Bun.Transpiler.transform stack overflows error: expect(received).toThrow(expected) Expected substring: "Maximum call stack size exceeded" Received function did not throw Received value: "let counter = 0;\nfor (let i = 0;i ..." ``` It reproduces on unrelated PRs (checked ~10 recent PR builds — all carry the identical annotation on a Windows lane) and doesn't show on `main`'s own CI because main builds don't run the Windows test lanes. #31333 removed the parser's fixed `MAX_STMT_DEPTH` cap; recursion is now bounded only by `StackCheck`. The old `lots-of-for-loop.js` fixture is ~15k levels deep, which still exhausts the ~8 MB main stacks on the Linux/macOS lanes but fits comfortably inside the 18 MiB stack reserve bun links with on Windows (`/STACK:0x1200000`), so the transpile now succeeds there and `toThrow` fails. ### Fix Per review: keep the assertions exactly as they are and make the fixture big enough that it overflows everywhere. - `fixtures/lots-of-for-loop.js` is regenerated from 15,112 to **320,000 nested `for` loops** (~9 MB, same shape: `let counter = 0;` → nested fors → `counter++;` → `console.log(counter);`). Against Windows' 18 MiB reserve minus the 256 KiB `StackCheck` headroom, 320k levels need only ~58 bytes of stack per recursion level to trip the guard — far below any realistic parse/visit/print frame, and leaves the same margin against future codegen getting leaner. The smaller Linux/macOS stacks keep throwing exactly as before. - The two `Bun.Transpiler.*` tests get a `60_000` ms timeout (the value the neighboring deeply-nested subprocess tests already use): on Windows the parser now recurses through ~18 MiB of stack before the guard trips, which costs a few seconds of wall time; the default 5 s would be tight. No other changes; no comments added. ### Verification - `bun bd test test/bundler/transpiler/transpiler.test.js` on current `main` + this change: 157 pass / 0 fail; the three stack-overflow tests throw in 57–360 ms on Linux (debug build). - Same three tests pass against a release build as well. - The Windows lanes on this PR are the proof that 320k levels overflow the 18 MiB reserve; if they somehow still don't throw there, the fixture gets another bump.
2 participants
Problem
Fuzzing found a second transpiler stack overflow (
sig:SIGSEGV:nostack): ~600 nested{blocks crash the process.#31242 guarded the expression recursion (
visit_expr_in_out,print_expr, DCE helpers), but the statement recursion was left unguarded. Nested blocks stay underMAX_STMT_DEPTH(1000) inparse_stmt, then the visit pass recurses throughvisit_stmts → visit_and_append_stmt → s_block → visit_stmtswith no stack check — each level stacks several multi-KB frames, so a few hundred levels exhaust the thread's stack (reproduces at depth 800 on a debug build's 8 MB main stack; smaller stacks crash at 600):Fix
Guard the statement recursion the same way the expression recursion already is:
visit_and_append_stmtnow checksstack_check.is_safe_to_recurse()(plus thereported_stack_overflowfast-path) and reports "Maximum call stack size exceeded" instead of descending, mirroringvisit_expr_in_out.print_stmtandprint_if(which self-recurses forelse ifchains without passing throughprint_stmt) get the same guardprint_expr/print_bindingalready have, so a deep AST printed on a thread with less stack headroom errors instead of overflowing.MAX_STMT_DEPTH/parse_stmt_depthhard cap fromparse_stmt(review feedback): recursion depth in every phase is now governed byStackCheckalone, matching the Zig parser.hoist_symbolsthe same way: it walks the scope tree before the visit pass at the full depth the parser allowed, and was only kept safe previously by the now-removed cap (the 15k-deeplots-of-for-loop.jsfixture overflowed it in release builds otherwise).With this, every arbitrarily-nestable AST recursion (statements, expressions, bindings) is stack-checked in all three phases (parse, visit, print); deep inputs throw a catchable
Maximum call stack size exceedederror.Verification
New test
deeply nested statement blocks error instead of crashing the processintest/bundler/transpiler/transpiler.test.jstranspiles nested-block andelse if-chain shapes at depths 600/800/990 (below the parse-time cap, deep enough to overflow an unguarded visitor) in a subprocess and asserts it exits cleanly.bun bd test test/bundler/transpiler/transpiler.test.js→ 147 pass, 0 fail; the repro above now throwsMaximum call stack size exceeded.