ReviewOS

zig-utils/zig-js

Crash: deep runtime chains (flat, proxy chains, bound functions, IsArray) recurse without a stack check

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

Parent: #934 (robustness: deep structures crash instead of throwing RangeError) Related: #938, whose review found these; that fix covers only recursion that follows source nesting

Confirmed by execution

Each program builds a deep runtime structure iteratively (no deep source, no deep JS call stack), then performs one operation. Run with test262 --eval built at b895723 (ReleaseFast), and with node v24.4.1 as the oracle:

operationzig-jsnode
a.flat(Infinity) where a contains itselfSIGSEGVRangeError: Maximum call stack size exceeded
b.flat(Infinity) on a 300,000-deep arraySIGSEGVRangeError
c.flat(Infinity) on a 50-deep array (control)11
Object.getPrototypeOf(p), p a chain of 200,000 trap-less Proxy objectsSIGSEGVRangeError
Object.isExtensible(p), same chainSIGSEGVRangeError
Object.defineProperty(p, 'x', {value: 1}), same chainSIGSEGVRangeError
Object.prototype.toString.call(p), chain of 200,000 proxies over an arraySIGSEGVRangeError
f(), f bound 100,000 timesSIGSEGV7 (node runs it, at 300,000 links too)
({}) instanceof g, g bound 100,000 timesSIGSEGVRangeError

The whole operation runs under one native built-in call (or one instanceof), so stackGuard runs at most once, before the recursion starts. #938 guards recursion over source nesting; none of these involve deep source.

Recursion sites (from code reading; the table above is what was executed)

  • Array.prototype.flat: flattenInto (src/interpreter.zig:19624) <-> flattenIntoLen (19636), once per nested array level. Neither is a tail call, because each keeps temp roots under a defer. depth is a Number, so Infinity - 1 never stops a self-referential array.
  • Proxy forwarding without a trap: proxyGetProto (15031), proxyIsExtensible (15055), proxyPreventExt (15074) and proxySetProto (15097) recurse once per proxy link. So do builtins.zig applyDescriptor (1771, via [[DefineOwnProperty]]) and the [[GetOwnProperty]] forwarding.
  • IsArray over proxies: objectToStringIsArray (23667) recurses per link. It runs from Object.prototype.toString, ArraySpeciesCreate, concat's IsConcatSpreadable, JSON.stringify's replacer check and flat. Array.isArray (builtins.zig isArrayValue) is already iterative.
  • Bound functions: [[Call]]/[[Construct]] follow boundFunction targets recursively through callValueWithThisAtSite / constructNTAtSite. instanceof goes through ordinaryHasInstance (21935) -> instanceOf (21892) per bound link without crossing a call boundary.

Direction

  • Where the spec algorithm is recursive but the chain has no user-observable step between links (a trap-less proxy, a bound function, IsArray), a loop with a hop count can replace the recursion. A bound call then works as it does in node, and a proxy chain throws once the count exceeds what the stack could hold.
  • Where user code can run between levels (flat reads elements, which can hit getters), keep the recursion but check the stack with Interpreter.checkNesting (#938), so it throws the same catchable RangeError.

A regression case should build each structure iteratively in one process per shape. Several of them crash, and a crash in one would hide the rest.

9 days ago

Fixed in f2b655d.

Reproduced first, on the post-#938 engine (81b49b1, ReleaseFast) — every shape in the table above still crashed, so #938's source-nesting guard does not reach them. Each structure was built iteratively in its own process, with /usr/bin/time -l for peak RSS.

Fix, chosen per site by what the spec makes observable between two links:

  • flat reads elements, which can run a getter, so FlattenIntoArray stays recursive and is charged to the #938 stack floor. depth is a Number, so Infinity - 1 never terminates on a self-referential array.
  • Trap-less proxy forwarding crosses no JS call boundary, so proxyDepth now checks the stack floor as well as the call depth — covering [[GetPrototypeOf]], [[IsExtensible]], [[PreventExtensions]], [[SetPrototypeOf]] and the rest — plus applyDescriptor in builtins.zig, which re-enters itself for a trap-less [[DefineOwnProperty]].
  • IsArray, bound [[Call]]/[[Construct]] and OrdinaryHasInstance have nothing observable between links, so they walk the chain in one frame. [[Construct]] carries step 4 of 10.4.1.2 per link, and instanceof still repeats the observable @@hasInstance lookup per hop.

Before → after (test262 --eval, ReleaseFast, node v24.4.1 as oracle):

shapebeforeafternode
a.flat(Infinity), a contains itselfSIGSEGVRangeErrorRangeError
b.flat(Infinity), 300,000 deepSIGSEGVRangeErrorRangeError
c.flat(Infinity), 50 deep (control)111
Object.getPrototypeOf(p), 200,000 proxiesSIGSEGVRangeErrorRangeError
Object.isExtensible(p)SIGSEGVRangeErrorRangeError
Object.defineProperty(p, 'x', …)SIGSEGVRangeErrorRangeError
Object.prototype.toString.call(p)SIGSEGVRangeErrorRangeError
f(), 200,000 bindsSIGSEGV77
({}) instanceof g, 200,000 bindsSIGSEGVfalseRangeError

false is what OrdinaryHasInstance defines once the chain is walked to its innermost target and {}'s prototype chain is checked; node reports RangeError because V8 recurses per link.

Pre-fix crash thresholds, for sizing the regression case: bound chains between 32,000 and 64,000 links, flat between 30,000 and 60,000 levels, proxy chains above 60,000.

No conformance change. test/built-ins/Proxy 311, Function 509, Array 3,081, Object 3,411, Reflect 153, JSON 165, test/language/expressions 11,038, test/annexB 1,086 — all 0 fail and identical before and after. VM-witness: Function 509 agree, Proxy 311 agree, expressions 11,025 agree with the same 13 pre-existing divergences, all identical. Flips 0 cases. Focused suites: runtime 14 (new 15-check case covering every shape plus controls for bound argument order, the innermost bound this, and newTarget in both directions), vm 4, jit 393, frontend 26, concurrency 4.

Found while reproducing this: a bound chain stores a quadratic amount of name text — 16,000 binds cost 755 MB against node's 48 MB — filed as #942. The regression case resets each bound function's configurable name so it measures frames, not names.

9 days ago

Landed in f2b655d with the evidence above.

Sign in to comment on this issue.