ReviewOS

zig-utils/zig-js

Crash: tree-walker recursion without a JS call escapes stackGuard on deeply nested source

#938
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Parent: #934 Related: #937 (compiler), #936 (parser), #935 (flat chains)

The gap

Interpreter.stackGuard protects JavaScript function calls: each call is charged to depth and probed with stack_scan.nearLimit. But the tree-walker also recurses over AST depth inside a single call — nested statements, hoisting, Annex B scanning, destructuring, AST copying — and none of that is charged. So deeply nested source crashes the process from inside one function body, with no JS call involved.

Confirmed by execution

Reproduced with the current test262 --eval FILE tree binary. For the last two rows the frame on top of the stack at the crash is an incidental leaf (getIteratorFor, alloc) — whatever happened to be running when the guard page was hit. The recursion that consumed the stack is the cycle given.

source (tree mode)depthrecursion
{ { … x=1; } } blocks, sloppy10,000Annex B analysis: annex_b.Analysis.scanStatement via Interpreter.collectAnnexBLegacy
same, 'use strict' (skips Annex B)10,000Interpreter.hoistVarsIn > hoistVarNames > evalStatements
var [[[…z…]]] = o array pattern16,000bindPattern > destructureArray > bindPatternWithReference
class with a private field, method body nested 10,000 blocks10,000deepCopyNodes > deepCopyNode from buildClass

Blocks nest just as deeply in strict code, so fixing only the Annex B scan would leave strict code crashing in hoisting.

Chokepoints

From a mapping pass, unguarded:

  • Evaluation: beginNodeEvaluation (interpreter.zig:5662), bindPattern (15889), tdzBindPattern (4576), predeclareForLexicalPattern (7053), checkRestrictedGlobalLexicalPattern (4602).
  • Hoisting and analysis done at evaluation time: hoistVarsIn (7442), hoistPatternVars (7479), collectEvalVarNamesNode (4427), evalPatternVarNames (4458), collectPatternNames (7075), patternBindsName (10189), labeledStatementTargetsIteration (5323), annexbStmtMayNeedScan (7517), and annex_b.Analysis.scanStatement/appendPattern (annex_b.zig:121/54).
  • AST rewriting and copying: deepCopyNode (8388), rewritePrivateNamesInNode (8167).

Host value formatting — narrow

print(a) on a 300,000-deep nested array segfaults in every execution mode. This is not a general toString bug: String(a), ""+a, a.join() and [a].toString() all correctly throw RangeError: Maximum call stack size exceeded. in every mode. Only the host print path formats values without the guard. Embedders exposing a similar host logging function would hit the same gap.

Not yet confirmed by execution

gc.traceChunk and gc.relocateChunk (gc.zig:1755/1761) were flagged as recursive over structure depth, but no crash has been reproduced. They are listed so they are not lost, and are not claimed.

Design constraints

Charge each of these recursions to the existing depth and stackGuard — the same contract #515 used for JSON — so failures are the standard catchable RangeError, and preserve OutOfMemory, Throw and internal control-flow errors rather than relabelling them.

8 days ago

Findings from the #937 work (b895723) that belong here, each traced in code and the first two measured:

  • Member chains (a.b.b.b…) and assignment chains evaluate recursively in the tree-walker. On the #936 binary a 30,000-link member chain segfaulted when evaluated in tree mode, while the same chain inside a never-called function did not. node raises RangeError for member chains that long.
  • Unary, var-pattern and conditional chains that the parser accepts past 40,000 levels evaluate fine in tree mode inside a never-called function, but they are still unguarded when evaluated.
  • annexbListMayNeedScan (interpreter.zig, reached from makeFunction for every sloppy block body) recurses over statement nesting before bytecode admission runs. The #937 review traced it but did not produce a crash: parsing uses more stack per level than this scan, so it needs a function created deep in the native stack. It is already in this issue's chokepoint list.
  • gc.traceChunk/relocateChunk walk the nested FnTemplate chunk tree. After #937 that tree can only be as deep as the compiler accepts. A crash would need a GC-enabled context collecting near the stack limit, so it stays unconfirmed, as before.
8 days ago

Fixed in 81b49b1.

Reproduction (before): the tree-walker recursed once per source level inside a single JS call, so stackGuard (a call-depth counter) never saw it. test262 --eval on a deep if-chain died with SIGSEGV at 139 levels.

Fix: evaluation compares the stack pointer against the interpreter's floor (stack_scan.nestingStackFloor, #936) at beginNodeEvaluation and in the walkers that recurse outside it — patterns, hoisting, the Annex B pre-scan, private-name rewriting, deep copy — and raises the same uncatchable RangeError the parser (#936) and compiler (#937) raise. The floor is read lazily on the first check so a thread that inherits an interpreter measures its own stack; evaluateForDebugger re-measures because an inspector evaluates on the dispatching thread.

Before → after (ReleaseFast, M-series):

probebeforeafter
deep if-chain, tree modeSIGSEGV at 139OK 2047
deep if-chain, default modeSIGSEGV at 139OK 2047
eval matrix, 232 cells (7 depths × 2 modes)39 cells SIGSEGVthose 39 raise RangeError

The 4 remaining ok → RangeError cells are explained, not regressions: print of a 10,000-deep expression goes through JS ToString (node throws there too), and the class-private-block limit moves 3006 → 2968 (tree) / 10027 → 9872 (default), exactly the redzone.

No behavioral drift: test/language 23,648 pass / 0 fail, test/annexB 1,086 pass / 0 fail, test/built-ins 23,126 pass over 83 subtrees, 12 vm-witness subtrees identical — flips 0 cases. Focused suites on the committed source: runtime 13, concurrency 4 (including a new case that evaluates on a spawned thread), frontend 26, vm 4, jit 393.

Performance. Measured with 5 binaries interleaved over 25 rounds on a call/loop benchmark (fib(27) + 30× an object/array loop), plus two controls — an unused added field, and a same-size field reorder:

builddefault heavy best / mediantree best / median
before1.000 / 1.0001.000 / 1.000
fix, field after step_budget1.039 / 1.0321.019 / 1.023
control: unused field, same slot1.044 / 1.0321.006 / 0.998
control: field reorder, same size1.009 / 0.9921.020 / 1.019
fix as committed (field last, align(1))0.995 / 0.9861.019 / 1.014

The first placement's 3–4% default-mode cost was not the check — an unused field reproduced it exactly, while a same-size reorder was free. Growing the 8-byte-aligned group shifts depth and the flags below it; declaring the field last at alignment 1 leaves every existing offset alone and measures at parity. In tree mode the change sits on top of the no-op reorder control (1.019 vs 1.020), i.e. the per-node check is at or below the layout-noise floor of this benchmark.

Three fixes fell out of the new error paths: module body hoisting that throws is now recorded as the module's evaluation error instead of leaving it "evaluating" forever; collectForLexNames/collectPatternNames propagate allocation failure instead of silently dropping a bound name; print roots its arguments across ToString.

8 days ago

Landed in 81b49b1 with the evidence above.

Sign in to comment on this issue.