ReviewOS

zig-utils/zig-js

Robustness: deeply nested source segfaults instead of throwing RangeError

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

Parent: #493 Related: #515 (the same class, fixed for JSON only), #932 (where this surfaced)

A small source crashes the process

Every row was executed. zig-js is the current test262 --eval binary; exit -11 is a SIGSEGV kill with no output. Found while timing #932 at depth 8000, where both the pre- and post-fix binaries crashed identically — so it predates that fix.

shapezig-js @ 8000zig-js @ 16000zig-js @ 32000node @ 16000 and 100000
((…(1)…)) parentheses, top levelOKSIGSEGVSIGSEGVRangeError: Maximum call stack size exceeded
[[…[]…]] arrays, top levelOKSIGSEGVSIGSEGVRangeError: Maximum call stack size exceeded
{ { … } } blocks, top levelOKSIGSEGVSIGSEGVRangeError: Maximum call stack size exceeded
{ { … } } blocks, inside a functionOKSIGSEGVSIGSEGV—
destructuring catch, top levelOKSIGSEGVSIGSEGV—
destructuring catch, inside a functionSIGSEGVSIGSEGVSIGSEGV—

The parentheses case is 32 KB of ( and ) around a 1. An embedder evaluating untrusted source cannot catch this: it is a process crash, not a JavaScript exception, and it is the kind of input #493 exists to bound.

What is and is not established

  • Established: zig-js crashes where node throws a catchable RangeError, on every nesting shape tried, from a source linear in depth.
  • Established: the depth that crashes varies by shape — 8000 for destructuring catch in a function, between 8000 and 16000 for everything else. Shapes that do more native work per level overflow sooner, which is consistent with a fixed native stack and no depth guard.
  • Not established: which phase overflows. Parsing is the one thing every shape shares, so the recursive-descent parser is the likeliest first site, but the compiler (recursing through compileStmt, as #932's profile showed) and the tree-walker may each overflow independently at other depths. The fix must cover whichever phases can overflow, not just the first one found.

Precedent

#515 fixed exactly this class for JSON.parse: reject hostile nesting before the native stack overflows. No equivalent guard exists for the parser, compiler or evaluator — there is no depth or stack-limit check in src/parser.zig or src/compiler.zig.

Suggested order

  1. Pin the table above as a regression probe that asserts a catchable error instead of a crash, and find the overflowing frame per shape (a crash cannot be sampled; run under a debugger, or bisect depth with a ReleaseSafe build).
  2. Decide the contract: node reports RangeError: Maximum call stack size exceeded. Match that for evaluation depth, and pick an explicit, documented error for parse depth.
  3. Guard by measured native stack headroom rather than a fixed depth constant where possible, since the safe depth differs per shape and per build mode; re-run the #926/#928 frozen workloads to confirm the guard costs nothing on normal code.
9 days ago

Phases identified — three independent overflow sites, not one

Every row is a backtrace captured at the moment of the crash (lldb --batch -o run -k 'bt 60'), taken at the minimum crashing depth found by bisection. Minimum depth matters: at a depth where every shape crashes, the parser overflows before compilation even starts, which would hide any later phase that is actually weaker.

shapefirst crashphaserecursion per nesting level
((…1…)) parentheses, top level~8621parser, expressions7 frames: parsePrimary > parseExpression > parseAssignment > parseConditional > parseBinary > parseUnary > parsePostfix
[[…]] arrays, top level~9113parser, expressions8 frames: parseArrayLiteral > parseSpreadable > parseAssignment > … > parsePrimary
destructuring catch, top level~15019parser, statements4 frames: parseStatement > parseBlock > parseTry > parseStatementInner
plain catch, inside a function~15019parser, statementssame 4
{ { … } } blocks, top level and inside a function~14773compiler, using pre-scan2 frames: stmtListHasDisposableDecl <-> stmtHasDisposableDecl
destructuring catch, inside a function~7390compiler, codegen5 frames: compileStmtList > compileStmt > compileTry > compileStmt > compileHoistedStmtList

This corrects my report above

I wrote that the parser was the likeliest first site. For nested blocks it is not the parser at all — they overflow in the compiler's using-declaration pre-scan before the parser's own limit is reached. And nested destructuring catches inside a function overflow in codegen at roughly half the depth the parser survives. So a parser-only depth guard would have left two of the six shapes still segfaulting.

It also means a fixed depth cap cannot be tuned once: the stack cost per nesting level ranges from 2 to 8 frames across these shapes, and it will differ again in Debug builds (larger frames) and on worker threads (smaller stacks).

Existing mechanism to reuse

The interpreter already solves this for runaway JavaScript recursion: Interpreter.stackGuard calls stack_scan.nearLimit(stack_redzone), a stack-pointer probe against each thread's registered OS stack bounds, and throws a catchable RangeError: Maximum call stack size exceeded.. It only probes once a chain is already deep (stack_check_floor), and falls back to a conservative depth cap where bounds are unknown. That is the right mechanism here too — correct per thread and per build mode, and nearly free on shallow code. The parser and compiler simply never call it.

Next

Map every recursive AST walker before writing the guard — not only the three sites above. The post-parse walks (#928's lexical walk, the private-name walk, scanSuperAndArgs, collectVarNames), the tree-walker's evaluation of nested expressions within a single call (which stackGuard does not cover, since no JS call occurs), and any AST teardown are all potential fourth sites that simply have not been the weakest yet.

9 days ago

Scope: this is a class of bugs, now split by subsystem

A mapping pass over the whole engine found 274 recursive walker entries, 8 of them guarded. It was followed by execution: I reproduced every crash cited below myself rather than relying on the pass's reports. One report turned out narrower than stated: a nested-array toString crash happens only through the host print function, while every JavaScript-visible path already throws RangeError correctly.

The most important finding is that the worst cases are not nested at all:

valid programtree modenode
template literal with 2,000 substitutions (~8 KB)SIGSEGVruns
'a'+'a'+…, 8,000 termsSIGSEGVruns

The parser builds these left-deep chains in a loop, but the tree-walker and compiler recurse down them. Since node runs them, they need a different fix from genuine nesting.

Two fix strategies

  1. Flat chains that node runs — walk the left spine iteratively so the program works. A stack guard would only turn a crash into an error on valid code.
  2. Genuine nesting, where node throws RangeError: Maximum call stack size exceeded — charge each recursion level to stack_scan.nearLimit, the per-thread stack probe Interpreter.stackGuard and #515 already use.

Sub-issues

issuesubsystemfixheadline evidence
#935tree-walker and compiler, flat chainsiterate2,000-substitution template crashes tree mode; node runs it
#936parser and post-parse analysisguard8.6K-deep parentheses; 90 KB of !
#937bytecode compiler and its pre-scansguarddestructuring catch fails at ~7,390, half the parser's limit; nested blocks fail in the using pre-scan
#938tree-walker recursion inside a single callguard10K nested blocks crash hoisting in strict code and the Annex B scan in sloppy code
zig-utils/zig-regex#23regex compile and backtrackingiterate, then guarda 30,000-character pattern crashes; node runs it

Each sub-issue records its chokepoints, and marks which walkers are confirmed by a crash and which are only unbounded by reading. This issue stays open as the umbrella until they are resolved.

9 days ago

#935 fixed in d91ae16 — the flat-chain half of this umbrella. Long +, template, logical and comma chains now work instead of crashing (57 of 96 crashing runs → 0), with every left-spine walker converted, including two parser walks that only overflowed first in Debug builds.

One finding matters for the guard work that remains: the fix lowers the maximum genuinely nested depth by 1.6–3.3% in some modes (exact, reproducible thresholds are on #935). Measurement shows it is not caused by inlining the chain helpers — nested calls and arrays shift too, which points at eval's overall frame. That makes #936–#938 the right place to resolve it: a stack guard makes the exact overflow depth irrelevant, where frame-layout tuning in a 60,000-line function would be fragile.

9 days ago

The crash umbrella is complete and can close.

Implemented work:

  • #935 / d91ae16a: flat expression chains are walked iteratively, so valid long templates, binary/logical chains, and comma sequences run instead of overflowing.
  • #936 / 480516b2: parser and post-parse analysis recursion use per-thread stack floors and return RangeError.
  • #937 / b8957237: bytecode compilation and recursive pre-scans use the same stack-floor contract.
  • #938 / 81b49b1a: tree-walker, hoisting, Annex B, pattern, private-name, and copy recursion are guarded inside a single JavaScript call.
  • #940 / f2b655d4: deep runtime proxy, flat, IsArray, bound-call/construct, and OrdinaryHasInstance chains no longer overflow.
  • zig-utils/zig-regex#23 / 3e8654e: long flat regex compile/backtracking spines are iterative.
  • #942 / 497eb4c2: 100,000-link bound-name storage is linear (600,001 result at 84.1 MB peak RSS instead of >3 GB/killed), removing the memory failure that masked the deep-chain probes.

The child issues contain their before/after crash matrices and conformance comparisons. On current main, the full sharded unit suite reports 2,442 passed, 1 skipped, 0 failed, and 0 leaked; the #942 TSan concurrency gate is also clean.

Sign in to comment on this issue.