ReviewOS

zig-utils/zig-js

Crash: long flat expression chains segfault where node runs them (templates, string concatenation)

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

Parent: #934 Related: #493

Valid programs that node runs, and zig-js segfaults on

Every row was executed against node and the current test262 --eval binary in each execution mode. These are flat sources — no nesting in the text — so they are not adversarial nesting; a large template literal or a long generated string concatenation reaches them.

valid programsizetree modedefaultvmnode
template literal with 2,000 ${1} substitutions~8 KBSIGSEGVOKOKruns
template literal with 8,000 substitutions~32 KBSIGSEGVSIGSEGVSIGSEGVruns
'a'+'a'+… with 8,000 terms~32 KBSIGSEGVOKOKruns
1+1+… with 16,000 terms~32 KBSIGSEGVSIGSEGVSIGSEGVruns
o.a.a.a… member chain, 8,000 deep~16 KBSIGSEGVOKOKRangeError

The tree-walker row matters most: Context.scriptTreeWalkPolicy sends any source containing let , const , catch, switch and similar to the tree-walker, which covers most real code, and that is exactly the mode that fails first.

Mechanism

The parser builds a left-associative chain iteratively (the parseBinary loop), producing a left-deep tree ((a+b)+c)+d. Consumers then recurse down the left spine, one native frame per term:

  • tree-walker: eval > evalBinary > eval (confirmed by crash backtrace, about 3,475 iterations to overflow);
  • compiler: compileExpr > compileExpr at the .binary arm's b.left (confirmed by backtrace, about 12,000 frames);
  • a template literal desugars to a left-deep +/to-string chain and fails the same way.

Why this needs a different fix from the rest of #934

node runs these. A stack guard would only turn a crash into a RangeError on code node accepts — still an incompatibility. The fix is to walk the left spine iteratively: descend the .left links collecting the chain, evaluate or emit the leftmost operand, then fold the right operands in order. That preserves exact left-to-right evaluation order, coercion order and abrupt-completion order, which is what the recursive form does now. The member-chain row is the exception, since node throws RangeError there; it belongs with the guard work.

Scope

Binary operators including string concatenation, template literals, and the other left-associative shapes that the parser builds iteratively and that no test here exercised yet: logical &&, || and ?? chains, and comma sequences. Each must be measured and pinned, not assumed.

9 days ago

Fixed in d91ae16

Every walker that recursed down a chain's left spine now iterates it. node and JavaScriptCore run these programs, so the goal was to make them work, not to turn the crash into an error.

It took ten walkers, not two

I started by converting only the evaluator and the compiler, the two that failed first in ReleaseFast. The first Debug run disproved that scoping: the parser's private-name walk overflowed first there, because Debug frames are several times larger. A fix that works in one build mode and crashes in another isn't a fix, so I searched for every function that passes a binary, logical or comma node's left operand to any call. That found ten, and no hidden mutual recursion:

filewalkers
tree-walkereval, deepCopyNode, rewritePrivateNamesInNode
compilercompileExpr, nodeHasYield, nameRefInClosure
parsercheckPrivateUsesInNode, scanSuperAndArgs
contextnodeHasTopLevelAwait
vmappendQuickArgumentsExpression

ast.ChainSpine records a chain's links top-down and yields them bottom-up, which is exactly the order a recursive walk reaches each right operand. So visit order, first-error order, mutation order and copy order are all unchanged.

Order-sensitive details, each checked against the code rather than assumed

  • Evaluation bookkeeping. beginNodeEvaluation does real work per node: the step budget, debugger stops, VM traps, GIL yield and GC safepoints. The evaluator calls it for each link top-down before any operand runs, as nested eval calls did.
  • Private brand checks. #x in o evaluates only its right operand, so it is never folded as a chain link.
  • nameRefInClosure can be visited in any order. I confirmed this by reading each query mode rather than assuming it, because its .call arm can return true for a direct eval. In recording mode, though, directEvalReferenceMatches marks every name captured and returns false, so recording never short-circuits.
  • The quick-arguments path accepts at most 16 nodes, so it now refuses deeper trees before recursing. The op limit would have refused them anyway.

Results

beforeafter
8 chain shapes × sizes 2K/8K/16K/32K × tree/default/vm57 of 96 runs SIGSEGV0 of 96, every shape's value identical across modes

The shapes are +, string concatenation, template literals, &&, ||, ??, comma and < chains. I also checked the target engine: JavaScriptCore runs < and == chains at 8,000, even though V8 throws there, so no operator was carved out.

One real cost, measured exactly

The largest genuinely nested expression that fits before a crash is slightly lower in some modes. These thresholds are fully reproducible (0 spread across repeated runs):

shapetreevm
1+(1+(1+…))3,475 → 3,407 (−2.0%)unchanged
f(f(f(…)))2,879 → 2,833 (−1.6%)12,030 → 11,629 (−3.3%)
[[[…]]]3,441 → 3,374 (−1.9%)unchanged

Two corrections to my own working are worth recording. An earlier measurement put this at −4%, but its bisection stopped at a 200-level window, and exact bisection gives about 2%. I also expected noinline on the spine-holding functions to remove it; measurement shows it changes nothing, so the helpers keep noinline only as a guarantee. The shift also shows up for nested calls and arrays, which involve no chains at all, which points at eval's overall frame rather than the chain code. These inputs crash either way, and the stack guard in #936–#938 is what turns them into a catchable RangeError at any depth.

Short chains in hot loops, the common case that now pays a link check, stay within the ±3% noise measured on the bytecode path, which is identical between the two builds.

Gates

  • test-runtime in Debug, 10/10, including a new case: 20,000-term chains, a 10,000-substitution template, left-to-right order observed through side effects, short-circuiting for all three logical operators, and a private brand check inside a chain. I checked every expectation against node before relying on it, and that caught one wrong expectation of mine: a 20,000-term < chain is false, not true.
  • Parser units 203 passed; compiler units 313 passed.
  • --vm-witness-subtree over 39 subtrees, which runs both tiers per test: every operator family, object and assignment expressions, calls, arrows, for, eval and function code, Annex B and both class trees. 13,773 tests, with divergence sets identical to the pre-change binary.

Sign in to comment on this issue.