bytecompiler: guard recursion in Array/ObjectPatternNode::bindValue - #355
bytecompiler: guard recursion in Array/ObjectPatternNode::bindValue#355robobun wants to merge 1 commit into
Conversation
…::bindValue
A deeply nested array destructuring pattern like
let [[...[[z]]...]] = w;
crashes the process with SIGSEGV on a release build. The parser's
parseDestructuringPattern guards its recursion with failIfStackOverflow(),
but once parsing succeeds, BytecodeGenerator walks the resulting AST via
ArrayPatternNode::bindValue, which recurses through
emitTryWithFinallyThatDoesNotShadowException and a ScopedLambda into the
nested pattern's bindValue with no stack check at all. Each level costs
roughly 1 KB of stack (iterator/done/value temporaries, CallArguments,
FinallyContext, the try-data wrapper), so a pattern a few thousand levels
deep runs past the guard page before the parser's own limit is reached.
Node.js throws RangeError for the same input.
Add the same isSafeToRecurse() check that emitNode() already performs to
the top of ArrayPatternNode::bindValue and ObjectPatternNode::bindValue.
On failure, set m_expressionTooDeep and return; BytecodeGenerator::generate
then reports ParserError::OutOfMemory and the caller sees a RangeError
instead of a crash. ObjectPatternNode happens not to overflow today because
its per-level frame is smaller, but it is the same unbounded recursion and
gets the same guard.
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward stack-recursion guard following the existing emitNode() pattern.
What was reviewed:
- Confirmed the guard is byte-for-byte identical to the void-returning
emitNode/emitNodeInConditionContextchecks inBytecodeGenerator.h:506-509, 608-611. - Verified
generator.vm()(public,BytecodeGenerator.h:377) andemitThrowExpressionTooDeepException()(BytecodeGenerator.h:1293) are both accessible fromNodesCodegen.cpp. - Checked that early
return;frombindValueis safe — callers don't inspect any output, andemitThrowExpressionTooDeepExceptionsetsm_outOfMemoryDuringConstructionsogenerate()bails withParserError::OutOfMemory.
Extended reasoning...
Overview
The PR adds a 4-line stack-depth guard at the top of ArrayPatternNode::bindValue and ObjectPatternNode::bindValue in NodesCodegen.cpp. Deeply nested destructuring patterns (e.g. [[[[...z...]]]]) recurse through bindValue without going through emitNode(), so the existing recursion guard there is bypassed and the process crashes with a native stack overflow. The fix mirrors the exact idiom already used in four places in BytecodeGenerator.h (emitNode, emitNodeInConditionContext, emitDefineClassElements): check VM::isSafeToRecurse(), and on failure call emitThrowExpressionTooDeepException() and return early.
Security risks
None introduced. The change is purely defensive — it converts an uncontrolled native stack overflow (SIGSEGV, potential DoS vector via crafted input) into a catchable RangeError. No new attack surface, no data handling, no auth/crypto.
Level of scrutiny
Low. This is a mechanical application of an established JSC pattern to two more recursion entry points. The added code is identical in shape to BytecodeGenerator.h:506-509 (void-returning variant). generator.vm() is a public VM& accessor already used ~15 times in the same file, and emitThrowExpressionTooDeepException() is public. The [[unlikely]] annotation matches existing usage. There is no happy-path behavior change: isSafeToRecurse() only trips near the soft stack limit, and shallow destructuring is unaffected.
Other factors
- Early-returning from
bindValuewithout emitting the destructuring bytecode is safe becauseemitThrowExpressionTooDeepExceptionsets the generator's OOM flag, causingBytecodeGenerator::generate()to discard the CodeBlock and reportParserError::OutOfMemory— the partially-emitted bytecode is never executed. - The PR description documents manual verification across all destructuring entry points (
let/const,for-of,catch, function params,eval). - The bug-hunting system found no issues.
- 8 net lines added, no lines removed, single file — well within the threshold for a mechanical fix.
Preview Builds
|
1 participant
A deeply nested array destructuring pattern crashes the process with SIGSEGV on a release build:
Node.js throws
RangeError: Maximum call stack size exceededfor the same input.Cause
The parser's
parseDestructuringPatternguards its own recursion withfailIfStackOverflow()and can parse ~20k levels before bailing. Once parsing succeeds,BytecodeGeneratorwalks the AST viaArrayPatternNode::bindValue, which recurses throughemitTryWithFinallyThatDoesNotShadowExceptionand aScopedLambdainto the nested pattern'sbindValuewith no stack check. Each level costs roughly 1 KB of native stack (iterator /done/valuetemporaries,CallArguments,FinallyContext, the try-data wrapper), so a pattern ~8.5k levels deep blows throughmaxPerThreadStackUsageand past the OS guard page before the parser's own limit is reached.ObjectPatternNode::bindValuehas the same unbounded recursion; it happens not to overflow today only because its per-level frame is much smaller than the array path, so the parser limit trips first.Fix
Add the same
isSafeToRecurse()check thatemitNode()already performs to the top of bothbindValueimplementations. On failure, callemitThrowExpressionTooDeepException()and return;BytecodeGenerator::generatethen reportsParserError::OutOfMemoryand the caller sees a catchableRangeErrorinstead of a crash.Verification
Against a bun release build linked with this change, every entry point that previously segfaulted (
let/constdeclaration,for (let [[…]] of …),catch ([[…]]), function parameters,(0, eval)(…)) now throws aRangeErrorat all tested depths; shallow destructuring is unchanged.