parser: make stack-overflow failure sticky across save-point backtracking#297
parser: make stack-overflow failure sticky across save-point backtracking#297robobun wants to merge 2 commits into
Conversation
…king When the recursion limit fires in parseAssignmentExpression for a deeply nested object or array literal, each enclosing level retries the input as a destructuring pattern (parseAssignmentExpression) and again as a member expression (parseAssignmentElement). Both retry paths call restoreSavePoint, which clears m_errorMessage, so the stack check has to fire again at the next recursion entry. With two retry branches at every level the work becomes exponential in nesting depth, and the parser appears to hang with unbounded memory growth instead of surfacing the RangeError. m_hasStackOverflow already survives restoreSavePoint; this change makes failIfStackOverflow() consult it so the first overflow short-circuits every subsequent recursion entry and the failure propagates in O(depth).
|
Warning Review limit reached
Next review available in: 20 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesParser stack-overflow handling
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-reasoned one-line fix that turns a parser hang into a prompt RangeError.
What was reviewed:
- Confirmed
failWithStackOverflow()setsm_hasStackOverflowand nothing ever clears it;restoreSavePoint/swapSavePointForErroronly resetm_errorMessage, so the sticky check is sound. m_hasStackOverflowis initialized tofalseper-Parser-instance (Parser.cpp:146), so no cross-parse leakage.- Short-circuit ordering keeps
[[unlikely]]accurate and avoids theisSafeToRecurse()call once tripped.
Extended reasoning...
Overview
Single-line change to the failIfStackOverflow() macro in Source/JavaScriptCore/parser/Parser.cpp, adding m_hasStackOverflow || ahead of the existing !canRecurse() check. A three-line comment explains the rationale. No other files or logic touched.
Security risks
None introduced. This is effectively a DoS-class fix: previously, deeply nested object literals ({a:{a:...}} ~3000 deep) triggered exponential backtracking after the recursion limit was hit, hanging the process with unbounded RSS growth. The change makes the first overflow terminal so failure propagates in O(depth). It strictly reduces attack surface.
Level of scrutiny
Low-to-moderate. The diff is one boolean disjunct in a macro. I verified the invariants it relies on:
failWithStackOverflow()(Parser.cpp:48) is the only writer ofm_hasStackOverflowand only sets ittrue.restoreSavePoint(Parser.h:2082) andswapSavePointForError(Parser.h:2043) clearm_errorMessagebut never touchm_hasStackOverflow, which is exactly the gap this patch closes.- The Parser constructor initializes
m_hasStackOverflow = false(Parser.cpp:146) and Parser instances are per-parse, so stickiness cannot leak across unrelated parses. - Once real stack overflow is hit, there is no legitimate alternate production that could succeed by recursing again, so making it sticky cannot cause a false rejection of valid input.
Other factors
The PR description includes a clear reproduction, root-cause trace through parseAssignmentExpression → tryParseDestructuringPatternExpression → parseAssignmentElement → parseMemberExpression, and before/after verification at depths 2600/3000/100000 plus regression checks on normal destructuring/arrow/array patterns. The bug hunting system found no issues. The change is mechanical enough and its supporting invariants easy enough to verify that human review would not add signal.
…atrix ports.ubuntu.com has been unreachable for hours, and a single arm64 apt failure was cancelling all 12 Linux variants. Retry apt with a Launchpad-registered mirror on failure, and let the remaining matrix entries finish when one variant dies. Mirrors the same hardening already on the dylan/llvm-22 branch.
There was a problem hiding this comment.
I didn't find any issues, but the parser macro change — while minimal — alters behavior on a core JSC parse path, so it's worth a human sign-off.
What was reviewed:
- Verified
m_hasStackOverflowis only set byfailWithStackOverflow(), initialized tofalseper-Parser, and never cleared byrestoreSavePoint/swapSavePointForError— so the sticky check can only short-circuit an already-doomed parse, never reject a previously-accepted one. - Dockerfile retry loop: confirmed the trailing
dpkg -lguard fails the build if all 3 attempts exhaust without installing. fail-fast: falseadditions match the existingwindows-crossjob pattern.
Extended reasoning...
Overview
Three independent changes bundled together:
Parser.cpp(1 line + comment):failIfStackOverflow()now also checks the stickym_hasStackOverflowflag, so once the recursion limit trips, every subsequentfailIfStackOverflow()call fails immediately even after save-point backtracking has clearedm_errorMessage. Fixes an exponential-retry hang on deeply nested object literals..github/workflows/build-reusable.yml: addsfail-fast: falseto five matrix jobs (linux, linux-musl, macos-cross, freebsd, linux-android) so one variant's failure doesn't cancel siblings.Dockerfile: replaces the two-mirrorapt-get updatefallback with a 3-attempt retry loop over update+install combined, swapping to fallback mirrors (archive.ubuntu.com for amd64, ocf.berkeley.edu for arm64 ports) between attempts, plusAcquire::Retries "5"and a finaldpkg -lsanity check.
Security risks
None identified. The parser change only adds a fast-fail condition on a flag that is exclusively set by the existing overflow path; it cannot accept input that was previously rejected. The Dockerfile change adds a third-party mirror (mirrors.ocf.berkeley.edu) as an arm64 fallback — a Launchpad-registered mirror, and the packages are signed, so apt's signature check still gates what gets installed.
Level of scrutiny
The CI and Dockerfile changes are low-risk infrastructure resilience. The parser change is tiny and I traced its correctness (m_hasStackOverflow is per-instance, init-false at Parser.cpp:146, set only at Parser.cpp:48, never touched by restoreSavePoint/internalRestoreState in Parser.h:2082-2086), but it lives in JavaScriptCore's hot parse path where any behavioral edge deserves a maintainer's eyes rather than a bot approval.
Other factors
The PR description is thorough with a reproduction and root-cause trace. The two commits already appear on main in the local checkout, suggesting this may already be merged. No prior human or claude[bot] review on the thread; coderabbit was rate-limited. No tests added, but the fix is a guard-condition tightening that's hard to regression-test without a stack-depth-dependent harness.
Preview Builds
|
1 participant
Reproduction
Before: hangs forever at 99% CPU with monotonic RSS growth (never terminates).
After:
RangeError: Maximum call stack size exceeded.in <50ms, including at depth 100000.Cause
When
failIfStackOverflow()trips insideparseAssignmentExpressionfor a deeply nested{a:{a:...}}, the error propagates back to each enclosingparseAssignmentExpressionframe, whosemaybeAssignmentPatternbranch then callsswapSavePointForError(clearingm_errorMessage) and retries the same input viatryParseDestructuringPatternExpression. That path reachesparseAssignmentElement, which on failure of itsparseDestructuringPatternattempt callsrestoreSavePoint(clearing the error again) and retries viaparseMemberExpression, which re-entersparseAssignmentExpression. Two alternate-production retries at each nesting level multiply to exponential work; the observable result is an apparent hang and unboundedStringPrintStream/ arena allocation.Fix
m_hasStackOverflowis already set byfailWithStackOverflow()and is not touched byrestoreSavePoint. Checking it infailIfStackOverflow()makes the first overflow short-circuit every subsequent recursion entry (parseAssignmentExpression,parsePrimaryExpression,parseDestructuringPattern, ...) so the failure propagates in O(depth).Verification
Destructuring, arrow functions, assignment patterns, array destructuring, and shallow nested objects all continue to parse normally.