ReviewOS

zig-utils/zig-js

Crash: deeply nested source overflows the parser and its analysis walks

#936
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Parent: #934 Related: #515 (the same guard, applied to JSON), #935 (flat chains — a different fix)

Confirmed by execution

Each row is a crash I reproduced with the current test262 --eval binary, with the recursion taken from an lldb backtrace at the crash. node throws a catchable RangeError: Maximum call stack size exceeded for the nesting shapes it was run on.

sourcefirst crashrecursion
((…1…)) parentheses~8,621 deepparsePrimary > parseExpression > parseAssignment > parseConditional > parseBinary > parseUnary > parsePostfix
[[…]] array literals~9,113 deepparseArrayLiteral > parseSpreadable > parseAssignment > … > parsePrimary
!!!…0 unary chain90,000 (90 KB)parseUnary > parseUnary, one source byte per level
nested try { } catch statements~15,019 deepparseStatement > parseBlock > parseTry > parseStatementInner

Chokepoints

From a mapping pass over src/parser.zig — these are the functions every recursive parsing cycle passes through:

  • parseAssignment (parser.zig:3138) — the expression chokepoint: assignment, conditional, arrow, yield, parentheses, array/object literals, call arguments, computed keys, default values. It recurses into itself directly, so the guard belongs at its entry.
  • parseUnary (3519) — its self-loop (!, -, typeof, await, prefix ++) never passes through parseAssignment.
  • parseBinary (3475) — the right-associative ** self-loop bypasses both of the above.
  • parseNew (3740) — the new new new X cycle; parsePrimary is an equivalent alternative.
  • parseStatement (1891) — every statement cycle: blocks, if/else, labels, loops, switch, try, with, function and class bodies.
  • parseBindingTarget (2183) — nested destructuring patterns.
  • litToPattern (2105) and exprToTarget (2165) — cover-grammar refinement recursing over an already-parsed expression.

Post-parse analysis walks — unbounded, not yet the weakest site

These recurse over finished-tree depth with no guard. None crashed at the depths tried, because the parser or compiler overflowed first; checkPrivateUsesInNode survived 36,000 levels. They still need guards, because shapes that the parser builds without recursion (see #935) can hand them a deeper tree than parsing did:

scanSuperAndArgs (4783), scanSuperAndArgsInPattern (4754), addPatternNames (1476), collectVarNames (1161), recurseScope (1282), checkPrivateUsesInNode (5034), checkPrivateUsesInPattern (5001); in context.zig, nodeHasTopLevelAwait (9809) and modulePatternBoundNames (9987).

Design constraints

  • Report RangeError: Maximum call stack size exceeded, not SyntaxError, matching node and #515. Keep ParseError compact.
  • Use stack_scan.nearLimit rather than a fixed depth: stack cost per level ranges from 1 to 8 frames across these shapes, and differs again in Debug and TSan builds and on worker threads with smaller stacks. Fall back to a conservative depth only where boundsKnown() is false.
  • Don't depend on the caller having run stack_scan.enter: call registerThreadBounds() when the parser is constructed, so syntax-only parses and parser unit tests are protected too.
  • Test in a subprocess — the test-runtime suite in tools/focused_engine_tests.zig from #515 — since a crash kills the test runner. Include a moderate valid depth that must still parse.
9 days ago

Fixed in 480516b

Deeply nested source now raises RangeError: Maximum call stack size exceeded. instead of killing the process, matching node. The lexer, the parser and the post-parse analysis walks check the stack before each recursion that follows source nesting.

The check is one comparison per level

A Parser reads its thread's stack floor once, when it is created: the OS stack limit plus the same 256 KiB redzone stackGuard keeps (1 MiB under TSan). Each guarded level then compares the stack pointer with that floor. Template sub-parsers inherit the floor.

The first version counted depth instead, and measurement rejected it. It cost +3.5% to +12.4% on the frontend benchmarks (nested_functions_1024 was the worst). I built a control with the counter compiled out, and it showed that the counter's increments, decrements and defers were most of the cost. The address comparison replaced it.

parseUnary and parseBinary recurse without bound only through a prefix operator's operand and a binary right operand, so the check sits on those two call sites, not on every operand.

The error has to survive every path to the caller

I ran a read-only adversarial review of the diff. It found five paths that turned StackExhausted into something else. I reproduced each one with the pre-fix binary before changing anything:

pathbefore the fix
for await (a[((…))] of …)SyntaxError: UnexpectedToken, because parseFor swallowed the target attempt's error and reparsed the head
nested for (a[function(){ … }()] of …) headsthe same swallow retried the exhausted descent at every enclosing head
GeneratorFunction / AsyncGeneratorFunction parametersSyntaxError: Function: invalid parameters or body, from the pre-lex that looks for yield
eval's post-parse context scansevery failure reported as the super SyntaxError; evalDeclaresArguments turned it into false
dynamic import()SyntaxError: Cannot parse module
private module loader (JSC__JSModuleLoader__evaluate)SyntaxError: Cannot load or evaluate module

All of these now report the RangeError.

Results (binaries built from the parent commit and from this one)

checkbeforeafter
18 shapes at 200,000 levels × top-level, eval/tree, eval/vm, plus GeneratorFunction and AsyncGeneratorFunction parametersevery shape SIGSEGVevery cell RangeError (top-level --eval prints StackExhausted, how Context.evaluate reports any parse failure)
import() of a 200,000-deep module, run as a corpus testrunner crashed (exit 139)2/2 pass
test262 --diag, 23 subtrees24,403 tests, 0 failures24,403 tests, 0 failures, identical output
frontend parse benchmarks, 12 workloads × 9 interleaved rounds × 15 iterations—median after/before 0.990 to 1.022; allocation counts and bytes identical

Shapes: parentheses, arrays, objects, !, delete, **, new, arrows, class heritage, blocks, if, labels, binding patterns, assignment patterns, templates, member chains, for await heads, nested for-of heads.

Gates: parser units 208 passed, including a test that parses all 18 shapes at depth 64 and requires StackExhausted at 200,000. test-runtime passed 11/11 in Debug. The Home private-value fixture passed 392/392 symbols, with a new check that a deep module rejects with the RangeError. (That fixture had not compiled since 0e3e248; 20b15b5 fixes it separately.)

Not fixed here

  • The compiler (#937) and the tree-walker (#938) still crash on trees the parser accepts. I measured this on the new binary: blocks, assignment chains, arrows and unary chains crash between roughly 7,000 and 60,000 levels. A 30,000-link member chain crashes both tiers; node throws RangeError for it.
  • Regex literals recurse inside zig-regex. The review traced /[[[…]]]/v to parseSetExpr, which zig-utils/zig-regex#23 already lists.
  • Parentheses, object literals and for await heads at 200,000 levels take 5 to 13 s to fail, while arrays take 0.0 s. That looks like a lookahead that is quadratic in nesting depth. It is older than this change, and I will measure it before filing.
  • Valid nested classic for (a[function(){ … }()]; …;) heads parse in exponential time and memory. On the parent commit: depth 16 used 116 MB, 18 used 431 MB, 20 took 821 MB and 0.75 s, and 22 went past 3 GB. node parses depth 200 in 0.2 ms. This is older than #936 and will be filed separately.

Sign in to comment on this issue.