js_parser: guard stack depth when parsing nested JSX elements#31537
Conversation
parse_jsx_element recurses directly for each nested child element (<a><b><c>...), but unlike the other recursive parse entry points it never consulted the parser's stack guard. A source like "() => <div>" repeated thousands of times nests that many <div> children (the "() =>" between each pair is parsed as JSX text), so the unbounded recursion ran off the end of the stack and the process died on the guard page with a bare SIGSEGV. Add the standard is_safe_to_recurse() check at the top of parse_jsx_element; the parse entry points already convert the resulting StackOverflow error into a "Maximum call stack size exceeded" diagnostic.
StatusFixes a fuzzer-found stack overflow (SIGSEGV, exit 139) in the TSX parser on deeply-nested
Waiting on CI. |
|
Warning Review limit reached
More reviews will be available in 19 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 11:21 PM PT - May 28th, 2026
❌ @robobun, your commit e99279b has some failures in 🧪 To try this PR locally: bunx bun-pr 31537That installs a local version of the PR into your bun-31537 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward stack-guard following the established is_safe_to_recurse() pattern; the two inline nits are cosmetic and non-blocking.
Extended reasoning...
Overview
This PR adds a 3-line stack-depth guard at the top of parse_jsx_element in src/js_parser/parse/parse_jsx.rs, plus a regression test. The guard is the standard if !p.stack_check.is_safe_to_recurse() { return Err(err!("StackOverflow")); } idiom already used verbatim in parse_property, parse_stmt, parse_binding, parse_typescript, parse_skip_typescript, and several visit/scan passes. The err!("StackOverflow") value is already caught by parse_entry.rs (lines 455/848) and converted into a graceful Maximum call stack size exceeded diagnostic, so no new error-handling plumbing is introduced.
Security risks
None — this is a hardening change that converts a fuzzer-found SIGSEGV (stack guard-page crash) into a catchable error. It strictly reduces attack surface; the guard only fires near stack exhaustion and cannot affect valid input.
Level of scrutiny
Low. The production change is mechanical pattern-following with zero ambiguity: the same three lines appear 8+ times elsewhere in src/js_parser/. The test is isolated (spawns a child process so a regression fails assertions rather than crashing the runner) and mirrors the existing transpiler-stack-overflow.test.ts sibling. No CODEOWNERS cover these paths.
Other factors
The two inline findings are explicitly nits in test code: a no-op path.join("input.tsx") and a String.repeat vs Buffer.alloc style preference (where the sibling test already uses .repeat). Neither affects correctness, behavior, or CI reliability, so they don't block approval. The PR description confirms existing JSX/transpiler suites pass and valid nested JSX is unaffected.
UpdateAddressed the two review nits from @claude in 6c2bc6f (test-only): inlined the Both review threads resolved. |
There was a problem hiding this comment.
LGTM — thanks for addressing the test nits; the guard matches the established pattern across the other recursive parse entry points.
Extended reasoning...
Overview
The production change is a 3-line guard at the top of parse_jsx_element in src/js_parser/parse/parse_jsx.rs: it checks p.stack_check.is_safe_to_recurse() and returns Err(err!("StackOverflow")) if not. The accompanying new test (test/bundler/transpiler/jsx-deep-nesting-stack-overflow.test.ts) spawns a child process that transpiles 50k-deep () => <div> and asserts the child is not killed by a signal and reports the call-stack error.
Security risks
None introduced. This is pure defensive hardening: it converts a fuzzer-found stack-guard-page SIGSEGV into a catchable SyntaxError via the existing err!("StackOverflow") handling in parse_entry.rs (lines 455/848). It strictly reduces the attack surface for pathological input.
Level of scrutiny
Low. The added guard is byte-for-byte identical to the pattern already used in parse_property.rs:256, parse/mod.rs:75 and :981 (parse_binding), parse_skip_typescript.rs:64/:247, parse_typescript.rs:221, and parse_stmt.rs:1898. The error sentinel is caught and converted to a user-facing "Maximum call stack size exceeded" diagnostic at the existing parse entry points. Valid JSX is unaffected — the guard only fires near actual stack exhaustion. The test is isolated in a subprocess so a regression fails assertions rather than crashing the runner.
Other factors
I left two minor nits on the previous revision (no-op path.join + unused import, and .repeat() → Buffer.alloc per test/CLAUDE.md). Both were addressed in 6c2bc6f and the threads are resolved; the current diff reflects the fixes. No CODEOWNERS cover src/js_parser/. The bug-hunting system found no issues on this revision.
CI noteThe earlier failure on build #58811 was on a single unrelated lane — This diff only changes the TSX parser (
Pushed one |
There was a problem hiding this comment.
LGTM — both earlier nits addressed; the guard matches the established stack_check pattern used across the other recursive parse entry points.
Extended reasoning...
Overview
The production change is three lines: a stack_check.is_safe_to_recurse() guard at the top of parse_jsx_element in src/js_parser/parse/parse_jsx.rs, returning err!("StackOverflow") on near-exhaustion. This is byte-for-byte the same idiom already present in parse_property, parse_binding, parse_stmt, parse_typescript, and p.rs — the JSX child-recursion path was simply the one entry point that lacked it. The accompanying test (jsx-deep-nesting-stack-overflow.test.ts) spawns a child process to transpile 50k-deep nested JSX and asserts it exits cleanly with the call-stack diagnostic rather than being killed by SIGSEGV.
Security risks
None introduced. This is a hardening change that removes a crash vector: pathological/fuzzed input that previously SIGSEGV'd the process now produces a catchable SyntaxError. The guard is a pure early-return; it cannot affect parsing of valid input since it only fires near actual stack exhaustion, far beyond any realistic JSX nesting depth.
Level of scrutiny
Low. The fix is mechanical pattern-application with abundant in-tree precedent, and the failure mode it addresses (unbounded recursion → guard-page fault) is well understood. The test is isolated in a subprocess so a regression fails the assertion rather than crashing the runner. No CODEOWNERS entry covers src/js_parser/.
Other factors
I previously left two non-blocking nits on the test file (no-op path.join and .repeat() vs Buffer.alloc); both were addressed in 6c2bc6f and the threads are resolved — the current diff reflects those fixes. The bug-hunting system found no issues this round. The one CI failure was an unrelated Windows package-install flake (same-OS non-baseline lane and all ASAN lanes passed), and a retrigger was pushed.
CI note (build #58825)This run's "failures" are Buildkite infra, not test failures: the Nothing here touches this PR — the diff is the TSX parser guard ( I've already spent my one CI re-roll, and there's no settled failure to re-roll anyway (the affected macOS lanes are just re-queued). The change is green on everything that's run; it needs a maintainer to merge once the re-queued macOS lanes finish. |
2 participants
Summary
The TSX parser SIGSEGVs on deeply-nested JSX. Found by fuzzing: a source like
() => <div>repeated thousands of times crashes with a bareSIGSEGV(exit 139) and no crash-handler output — it dies on the stack guard page.Repro
Cause
parse_jsx_element(src/js_parser/parse/parse_jsx.rs) recurses directly for every nested child element (<a><b><c>...) at theT::TLessThanchild branch. Every other recursive parse entry point (parse_expr_common,parse_stmt,parse_property,parse_binding, the TypeScript skippers) consults the parser'sstack_check, but this JSX child-recursion path never did.The fuzzer input
() => <div>() => <div>...nests that many<div>children — the() =>between each pair is lexed as JSX text (TStringLiteral) — so the recursion depth grows unbounded and runs off the end of the stack.Fix
Add the standard guard at the top of
parse_jsx_element:The parse entry points (
parse_entry.rs) already catcherr!("StackOverflow")and turn it into a gracefulMaximum call stack size exceededdiagnostic, so the transpiler now reports a catchableSyntaxErrorinstead of crashing. Valid nested JSX is unaffected — the guard only fires near stack exhaustion, well past any realistic nesting depth.Verification
test/bundler/transpiler/jsx-deep-nesting-stack-overflow.test.ts: spawns a child that transpiles 50k-deep() => <div>and asserts it is not killed by a signal and reports the call-stack error.SIGSEGV→ test fails.Maximum call stack size exceeded→ test passes.transpiler.test.js: 166 pass / 0 fail;scope-mismatch-panic.test.ts: all pass).<a><b><c>hi</c></b></a>) still transpiles correctly.