ReviewOS

zig-utils/zig-js

Crash: deeply nested source overflows the bytecode compiler and its pre-scans

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

Parent: #934 Related: #936 (parser), #935 (flat chains), #932 (a compiler quadratic on the same shapes)

Confirmed by execution

Reproduced with the current test262 --eval binary; recursion from an lldb backtrace at the crash. In several rows the parser succeeds and the compiler is the first phase to overflow, so a parser-only guard would not prevent these.

sourcefirst crashrecursion
destructuring catch nested in a function~7,390 deepcompileStmtList > compileStmt > compileTry > compileStmt > compileHoistedStmtList
{ { … } } blocks, top level or in a function~14,773 deepstmtListHasDisposableDecl <-> stmtHasDisposableDecl — the using-declaration pre-scan, which overflows before code generation starts
a=a=… assignment chain15,000compileExpr > compileNamedExpr; parses fine
x=>x=>… arrow chain10,000shadowAdd > functionBindingInventory > functionNeedsTdzChecks > compileFunction; parses fine
!!!… unary chain30,000compileExpr > compileExpr; parses fine

The destructuring-catch row fails at roughly half the depth the parser survives, so the compiler's per-level stack cost is the binding constraint for that shape.

Why this is broad

A mapping pass found 40 recursion chokepoints and no existing guard in compiler.zig. They fall into two groups:

  • Code generation dispatchers: compileStmt (3172), compileExpr (5155), compilePattern (4178), compileTailExpr (4552), compileOptionalValue (4924), compileFunction (6350).
  • Independent pre-scans, each re-walking full subtree depth before code generation reaches it: stmtHasDisposableDecl/stmtListHasDisposableDecl (1517/1526), stmtContainsDisposableDeclDeep (1531), nodeHasYield (802), nameRefInClosure (1222), collectRepeatedBodyBindings (1136), shadowScanStmt (1871), tdzScanStmt (1983), collectFunctionLocals (6714), stmtCanEscapeAbruptly (1389), the pattern predicates (patternBindsName 626, collectPatternBindingNames 1066, patternHasEvaluationExpressions 1102), and the environment-lexical pattern emitters (2650–2923).

Guarding only the dispatchers leaves the pre-scans: the blocks row above crashes in a pre-scan while compileStmt has plenty of headroom.

Design constraints

  • Surface RangeError: Maximum call stack size exceeded. It must not come back as error.Unsupported: that silently falls back to the tree-walker, which then recurses over the same tree and crashes in evaluation instead (see the tree-walker issue).
  • Use stack_scan.nearLimit with a depth floor, as Interpreter.stackGuard does, so shallow code pays a single comparison.
  • Re-measure the #926 and #928 frozen workloads and the #932 curve after adding guards, to show they cost nothing on normal code.
9 days ago

Fixed in b895723

The bytecode compiler and its pre-scans now stop at the same stack floor as the parser (#936). A tree that parses but is too deep to compile raises RangeError: Maximum call stack size exceeded. instead of segfaulting.

Where the check goes

  • Code generation. A Compiler reads the floor once, when compilation begins. Every nested function compiler inherits it, including generators and async functions. Checks sit in compileStmt, compileExpr, compilePattern, compileOptionalValue, compileTailExpr and the environment-lexical walkers.
  • Pre-scans. As the issue predicted, checking only the dispatchers would not have been enough: the pre-scans walk whole subtrees before code generation reaches them. About 30 of them now take the floor and return an error union: nodeHasYield, nameRefInClosure, the disposable-declaration, TDZ and shadow scans, the binding collectors, collectFunctionLocals, the repeated-body and pattern predicates, and annex_b.collect.

Surfacing: a RangeError, never a fallback

  • CompileError gains StackExhausted. Admission passes it through rather than turning it into a rejection, which would fall back to the tree-walker.
  • Script admission used to report every admission error as OutOfMemory; it now raises the RangeError.
  • Function creation (generator, async and plain bytecode admission) and the interpreter's Annex B scan raise the RangeError. Agent scripts no longer fall back.
  • Module function instantiation runs on a separate interpreter outside evalModule, and its throw never reached the context. A read-only review found this, and I reproduced it: import() of such a module rejected with SyntaxError: Cannot parse module before the throw was surfaced. The same review found that nested generator/async compiles re-read the floor instead of inheriting it; that is fixed too.

Results (binaries from the parent engine and from this change)

checkbeforeafter
compile-only matrix: 16 shapes × depths 1,000–200,000 × default/vm/tree, in never-called functions32 of 384 cells SIGSEGVthose 32 raise RangeError; the other 352 are unchanged
import() / static import / self-declaring module with a too-deep generator bodyrunner crashed (exit 139)RangeError
--vm-witness-subtree, 12 subtrees22,316 testsdivergence sets identical
--diag, 18 subtrees24,067 tests, 0 failures0 failures, identical output
9 compile benchmarks + the #926/#928 parser rows, 9 interleaved rounds × 15 iterations—median after/before 0.984 to 1.022; allocations identical

The shapes that crashed only when compiled were blocks, destructuring catch parameters, assignment chains, arrows, unary chains, if-chains, member chains, var patterns, conditionals and labels.

Pinned test. A new test-runtime case sweeps 7 shapes × 3 wrappers (generator body, nested plain function, loop body) to depth 128,000. Every depth must either work or throw the RangeError. For the four shapes the parser walks more cheaply than the compiler, the case binary-searches for the deepest source that parses inside an unexecuted block and requires that creating it throws. That check fails if exhaustion ever becomes a rejection again. It crashes the parent binary (exit 139).

Left for #938. Tree-walker evaluation recursion, including annexbListMayNeedScan at function creation, which the review traced to the path before admission.

Sign in to comment on this issue.