Skip to content

Commit 3321ead

Browse files
wyrichtewyrichte
committed
[MERGE #5739 @wyrichte] Fixes #5699 - Converts recursive loop to iterative in MayHaveSideEffectOnNode to avoid a stack overflow.
Merge pull request #5739 from wyrichte:build/wyrichte/stack_overflow_1
1 parent ed98335 commit 3321ead

File tree

3 files changed

+112
-51
lines changed

3 files changed

+112
-51
lines changed

‎lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Lines changed: 73 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@ if ((isTopLevel)) \
3434
byteCodeGenerator->EndStatement(pnode); \
3535
}
3636

37-
BOOL MayHaveSideEffectOnNode(ParseNode *pnode, ParseNode *pnodeSE)
37+
BOOL MayHaveSideEffectOnNode(ParseNode *pnode, ParseNode *pnodeSE, ByteCodeGenerator *byteCodeGenerator)
3838
{
39-
// Try to determine whether pnodeSE may kill the named var represented by pnode.
39+
// Try to determine whether pnodeSE (SE = side effect) may kill the named var represented by pnode.
4040

4141
if (pnode->nop == knopComputedName)
4242
{
@@ -49,54 +49,76 @@ BOOL MayHaveSideEffectOnNode(ParseNode *pnode, ParseNode *pnodeSE)
4949
return false;
5050
}
5151

52-
uint fnop = ParseNode::Grfnop(pnodeSE->nop);
53-
if (fnop & fnopLeaf)
54-
{
55-
// pnodeSE is a leaf and can't kill anything.
56-
return false;
57-
}
52+
ArenaAllocator *alloc = byteCodeGenerator->GetAllocator();
53+
SList<ParseNode*> pNodeSEStack(alloc);
5854

59-
if (fnop & fnopAsg)
60-
{
61-
// pnodeSE is an assignment (=, ++, +=, etc.)
62-
// Trying to examine the LHS of pnodeSE caused small perf regressions,
63-
// maybe because of code layout or some other subtle effect.
64-
return true;
65-
}
55+
pNodeSEStack.Push(pnodeSE);
6656

67-
if (fnop & fnopUni)
57+
// A pnodeSE can have children that can cause a side effect on pnode. A stack is used to check
58+
// pnodeSE and all potential pnodeSE children that could cause a side effect on pnode. When a
59+
// child pnodeSE can cause a side effect on pnode, immediately return true. Otherwise continue
60+
// checking children of pnodeSE until none exist
61+
while (!pNodeSEStack.Empty())
6862
{
69-
// pnodeSE is a unary op, so recurse to the source (if present - e.g., [] may have no opnd).
70-
if (pnodeSE->nop == knopTempRef)
63+
ParseNode *currPnodeSE = pNodeSEStack.Pop();
64+
uint fnop = ParseNode::Grfnop(currPnodeSE->nop);
65+
66+
if (fnop & fnopLeaf)
7167
{
72-
return false;
68+
// pnodeSE is a leaf and can't kill anything.
69+
continue;
7370
}
74-
else
71+
else if (fnop & fnopAsg)
7572
{
76-
return pnodeSE->AsParseNodeUni()->pnode1 && MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeUni()->pnode1);
73+
// pnodeSE is an assignment (=, ++, +=, etc.)
74+
// Trying to examine the LHS of pnodeSE caused small perf regressions,
75+
// maybe because of code layout or some other subtle effect.
76+
return true;
77+
}
78+
else if (fnop & fnopUni)
79+
{
80+
// pnodeSE is a unary op, so recurse to the source (if present - e.g., [] may have no opnd).
81+
if (currPnodeSE->nop == knopTempRef)
82+
{
83+
continue;
84+
}
85+
else if (currPnodeSE->AsParseNodeUni()->pnode1)
86+
{
87+
pNodeSEStack.Push(currPnodeSE->AsParseNodeUni()->pnode1);
88+
}
89+
}
90+
else if (fnop & fnopBin)
91+
{
92+
// currPnodeSE is a binary (or ternary) op, so check sources (if present).
93+
94+
pNodeSEStack.Push(currPnodeSE->AsParseNodeBin()->pnode1);
95+
96+
if (currPnodeSE->AsParseNodeBin()->pnode2)
97+
{
98+
pNodeSEStack.Push(currPnodeSE->AsParseNodeBin()->pnode2);
99+
}
100+
}
101+
else if (currPnodeSE->nop == knopQmark)
102+
{
103+
ParseNodeTri * pnodeTriSE = currPnodeSE->AsParseNodeTri();
104+
105+
pNodeSEStack.Push(pnodeTriSE->pnode1);
106+
pNodeSEStack.Push(pnodeTriSE->pnode2);
107+
pNodeSEStack.Push(pnodeTriSE->pnode3);
108+
}
109+
else if (currPnodeSE->nop == knopCall || currPnodeSE->nop == knopNew)
110+
{
111+
pNodeSEStack.Push(currPnodeSE->AsParseNodeCall()->pnodeTarget);
112+
113+
if (currPnodeSE->AsParseNodeCall()->pnodeArgs)
114+
{
115+
pNodeSEStack.Push(currPnodeSE->AsParseNodeCall()->pnodeArgs);
116+
}
117+
}
118+
else if (currPnodeSE->nop == knopList)
119+
{
120+
return true;
77121
}
78-
}
79-
else if (fnop & fnopBin)
80-
{
81-
// pnodeSE is a binary (or ternary) op, so recurse to the sources (if present).
82-
return MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeBin()->pnode1) ||
83-
(pnodeSE->AsParseNodeBin()->pnode2 && MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeBin()->pnode2));
84-
}
85-
else if (pnodeSE->nop == knopQmark)
86-
{
87-
ParseNodeTri * pnodeTriSE = pnodeSE->AsParseNodeTri();
88-
return MayHaveSideEffectOnNode(pnode, pnodeTriSE->pnode1) ||
89-
MayHaveSideEffectOnNode(pnode, pnodeTriSE->pnode2) ||
90-
MayHaveSideEffectOnNode(pnode, pnodeTriSE->pnode3);
91-
}
92-
else if (pnodeSE->nop == knopCall || pnodeSE->nop == knopNew)
93-
{
94-
return MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeCall()->pnodeTarget) ||
95-
(pnodeSE->AsParseNodeCall()->pnodeArgs && MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeCall()->pnodeArgs));
96-
}
97-
else if (pnodeSE->nop == knopList)
98-
{
99-
return true;
100122
}
101123

102124
return false;
@@ -9718,7 +9740,7 @@ void EmitJumpCleanup(ParseNodeStmt *pnode, ParseNode *pnodeTarget, ByteCodeGener
97189740
void EmitBinaryOpnds(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
97199741
{
97209742
// If opnd2 can overwrite opnd1, make sure the value of opnd1 is stashed away.
9721-
if (MayHaveSideEffectOnNode(pnode1, pnode2))
9743+
if (MayHaveSideEffectOnNode(pnode1, pnode2, byteCodeGenerator))
97229744
{
97239745
SaveOpndValue(pnode1, funcInfo);
97249746
}
@@ -9742,7 +9764,7 @@ void EmitBinaryReference(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator
97429764
switch (pnode1->nop)
97439765
{
97449766
case knopName:
9745-
if (fLoadLhs && MayHaveSideEffectOnNode(pnode1, pnode2))
9767+
if (fLoadLhs && MayHaveSideEffectOnNode(pnode1, pnode2, byteCodeGenerator))
97469768
{
97479769
// Given x op y, y may kill x, so stash x.
97489770
// Note that this only matters if we're loading x prior to the op.
@@ -9755,7 +9777,7 @@ void EmitBinaryReference(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator
97559777
// We're loading the value of the LHS before the RHS, so make sure the LHS gets a register first.
97569778
funcInfo->AcquireLoc(pnode1);
97579779
}
9758-
if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2))
9780+
if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2, byteCodeGenerator))
97599781
{
97609782
// Given x.y op z, z may kill x, so stash x away.
97619783
SaveOpndValue(pnode1->AsParseNodeBin()->pnode1, funcInfo);
@@ -9767,13 +9789,13 @@ void EmitBinaryReference(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator
97679789
// We're loading the value of the LHS before the RHS, so make sure the LHS gets a register first.
97689790
funcInfo->AcquireLoc(pnode1);
97699791
}
9770-
if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2) ||
9771-
MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode1->AsParseNodeBin()->pnode2))
9792+
if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2, byteCodeGenerator) ||
9793+
MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode1->AsParseNodeBin()->pnode2, byteCodeGenerator))
97729794
{
97739795
// Given x[y] op z, y or z may kill x, so stash x away.
97749796
SaveOpndValue(pnode1->AsParseNodeBin()->pnode1, funcInfo);
97759797
}
9776-
if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode2, pnode2))
9798+
if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode2, pnode2, byteCodeGenerator))
97779799
{
97789800
// Given x[y] op z, z may kill y, so stash y away.
97799801
// But make sure that x gets a register before y.
@@ -9883,12 +9905,12 @@ bool CollectConcat(ParseNode *pnodeAdd, DListCounted<ParseNode *, ArenaAllocator
98839905
void EmitConcat3(ParseNode *pnode, ParseNode *pnode1, ParseNode *pnode2, ParseNode *pnode3, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
98849906
{
98859907
byteCodeGenerator->StartStatement(pnode);
9886-
if (MayHaveSideEffectOnNode(pnode1, pnode2) || MayHaveSideEffectOnNode(pnode1, pnode3))
9908+
if (MayHaveSideEffectOnNode(pnode1, pnode2, byteCodeGenerator) || MayHaveSideEffectOnNode(pnode1, pnode3, byteCodeGenerator))
98879909
{
98889910
SaveOpndValue(pnode1, funcInfo);
98899911
}
98909912

9891-
if (MayHaveSideEffectOnNode(pnode2, pnode3))
9913+
if (MayHaveSideEffectOnNode(pnode2, pnode3, byteCodeGenerator))
98929914
{
98939915
SaveOpndValue(pnode2, funcInfo);
98949916
}

‎test/Miscellaneous/MayHaveSideEffectOnNodeSO.js

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
// MayHaveSideEffectOnNode() was a recursive function that could recurse enough times to cause
7+
// an uncaught stack overflow. This was fixed by converting the recursive loop into an iterative
8+
// loop.
9+
//
10+
// An example of a program that caused the stack overflow is:
11+
// eval("var a;a>(" + Array(2 ** 15).fill(0).join(",") + ");");
12+
//
13+
// MayHaveSideEffectOnNode() is originally called because the righthand side of the pNodeBin
14+
// ">" may overwrite the lefthand side of ">". The righthand side of pNodeBin is a deep tree
15+
// in which each pNode of the longest path is a pNodeBin(",").Since children of the original
16+
// pNodeBin -> right are pNodeBins themselves, MayHaveSideEffectOnNode() must check all of
17+
// their children as well.MayHaveSideEffectOnNode's original implementation was recursive and
18+
// thus the stack would overflow while recursing through the path of pNodeBins.
19+
20+
WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
21+
22+
var tests = [
23+
{
24+
name: "MayHaveSideEffectOnNode() should not cause a stack overflow nor should fail to \
25+
terminate",
26+
body: function () {
27+
eval("var a;a>(" + Array(2 ** 15).fill(0).join(",") + ");");
28+
eval("var a;a===(" + Array(2 ** 15).fill(0).join(",") + ");");
29+
}
30+
}
31+
]
32+
33+
testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

‎test/Miscellaneous/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,4 +118,10 @@
118118
<baseline>nullByte-string.baseline</baseline>
119119
</default>
120120
</test>
121+
<test>
122+
<default>
123+
<files>MayHaveSideEffectOnNodeSO.js</files>
124+
<compile-flags>-args summary -endargs</compile-flags>
125+
</default>
126+
</test>
121127
</regress-exe>

0 commit comments

Comments
 (0)