Fixes chakra-core#5699 - Converts recursive loop to iterative in MayH…

…aveSideEffectOnNode to avoid a stack overflow.
wyrichte · Oct 3, 2018 · eb57b65 · eb57b65
1 parent 831e6b2
commit eb57b65
Show file tree

Hide file tree

Showing 3 changed files with 112 additions and 51 deletions.
diff --git a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
@@ -34,9 +34,9 @@ if ((isTopLevel)) \
     byteCodeGenerator->EndStatement(pnode); \
 }
 
-BOOL MayHaveSideEffectOnNode(ParseNode *pnode, ParseNode *pnodeSE)
+BOOL MayHaveSideEffectOnNode(ParseNode *pnode, ParseNode *pnodeSE, ByteCodeGenerator *byteCodeGenerator)
 {
-    // Try to determine whether pnodeSE may kill the named var represented by pnode.
+    // Try to determine whether pnodeSE (SE = side effect) may kill the named var represented by pnode.
 
     if (pnode->nop == knopComputedName)
     {
@@ -49,54 +49,76 @@ BOOL MayHaveSideEffectOnNode(ParseNode *pnode, ParseNode *pnodeSE)
         return false;
     }
 
-    uint fnop = ParseNode::Grfnop(pnodeSE->nop);
-    if (fnop & fnopLeaf)
-    {
-        // pnodeSE is a leaf and can't kill anything.
-        return false;
-    }
+    ArenaAllocator *alloc = byteCodeGenerator->GetAllocator();
+    SList<ParseNode*> pNodeSEStack(alloc);
 
-    if (fnop & fnopAsg)
-    {
-        // pnodeSE is an assignment (=, ++, +=, etc.)
-        // Trying to examine the LHS of pnodeSE caused small perf regressions,
-        // maybe because of code layout or some other subtle effect.
-        return true;
-    }
+    pNodeSEStack.Push(pnodeSE);
 
-    if (fnop & fnopUni)
+    // A pnodeSE can have children that can cause a side effect on pnode. A stack is used to check
+    // pnodeSE and all potential pnodeSE children that could cause a side effect on pnode. When a
+    // child pnodeSE can cause a side effect on pnode, immediately return true. Otherwise continue
+    // checking children of pnodeSE until none exist
+    while (!pNodeSEStack.Empty())
     {
-        // pnodeSE is a unary op, so recurse to the source (if present - e.g., [] may have no opnd).
-        if (pnodeSE->nop == knopTempRef)
+        ParseNode *currPnodeSE = pNodeSEStack.Pop();
+        uint fnop = ParseNode::Grfnop(currPnodeSE->nop);
+
+        if (fnop & fnopLeaf)
         {
-            return false;
+            // pnodeSE is a leaf and can't kill anything.
+            continue;
         }
-        else
+        else if (fnop & fnopAsg)
         {
-            return pnodeSE->AsParseNodeUni()->pnode1 && MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeUni()->pnode1);
+            // pnodeSE is an assignment (=, ++, +=, etc.)
+            // Trying to examine the LHS of pnodeSE caused small perf regressions,
+            // maybe because of code layout or some other subtle effect.
+            return true;
+        }
+        else if (fnop & fnopUni)
+        {
+            // pnodeSE is a unary op, so recurse to the source (if present - e.g., [] may have no opnd).
+            if (currPnodeSE->nop == knopTempRef)
+            {
+                continue;
+            }
+            else if (currPnodeSE->AsParseNodeUni()->pnode1)
+            {
+                pNodeSEStack.Push(currPnodeSE->AsParseNodeUni()->pnode1);
+            }
+        }
+        else if (fnop & fnopBin)
+        {
+            // currPnodeSE is a binary (or ternary) op, so check sources (if present).
+
+            pNodeSEStack.Push(currPnodeSE->AsParseNodeBin()->pnode1);
+
+            if (currPnodeSE->AsParseNodeBin()->pnode2)
+            {
+                pNodeSEStack.Push(currPnodeSE->AsParseNodeBin()->pnode2);
+            }
+        }
+        else if (currPnodeSE->nop == knopQmark)
+        {
+            ParseNodeTri * pnodeTriSE = currPnodeSE->AsParseNodeTri();
+
+            pNodeSEStack.Push(pnodeTriSE->pnode1);
+            pNodeSEStack.Push(pnodeTriSE->pnode2);
+            pNodeSEStack.Push(pnodeTriSE->pnode3);
+        }
+        else if (currPnodeSE->nop == knopCall || currPnodeSE->nop == knopNew)
+        {
+            pNodeSEStack.Push(currPnodeSE->AsParseNodeCall()->pnodeTarget);
+
+            if (currPnodeSE->AsParseNodeCall()->pnodeArgs)
+            {
+                pNodeSEStack.Push(currPnodeSE->AsParseNodeCall()->pnodeArgs);
+            }
+        }
+        else if (currPnodeSE->nop == knopList)
+        {
+            return true;
         }
-    }
-    else if (fnop & fnopBin)
-    {
-        // pnodeSE is a binary (or ternary) op, so recurse to the sources (if present).
-        return MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeBin()->pnode1) ||
-            (pnodeSE->AsParseNodeBin()->pnode2 && MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeBin()->pnode2));
-    }
-    else if (pnodeSE->nop == knopQmark)
-    {
-        ParseNodeTri * pnodeTriSE = pnodeSE->AsParseNodeTri();
-        return MayHaveSideEffectOnNode(pnode, pnodeTriSE->pnode1) ||
-            MayHaveSideEffectOnNode(pnode, pnodeTriSE->pnode2) ||
-            MayHaveSideEffectOnNode(pnode, pnodeTriSE->pnode3);
-    }
-    else if (pnodeSE->nop == knopCall || pnodeSE->nop == knopNew)
-    {
-        return MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeCall()->pnodeTarget) ||
-            (pnodeSE->AsParseNodeCall()->pnodeArgs && MayHaveSideEffectOnNode(pnode, pnodeSE->AsParseNodeCall()->pnodeArgs));
-    }
-    else if (pnodeSE->nop == knopList)
-    {
-        return true;
     }
 
     return false;
@@ -9711,7 +9733,7 @@ void EmitJumpCleanup(ParseNodeStmt *pnode, ParseNode *pnodeTarget, ByteCodeGener
 void EmitBinaryOpnds(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
 {
     // If opnd2 can overwrite opnd1, make sure the value of opnd1 is stashed away.
-    if (MayHaveSideEffectOnNode(pnode1, pnode2))
+    if (MayHaveSideEffectOnNode(pnode1, pnode2, byteCodeGenerator))
     {
         SaveOpndValue(pnode1, funcInfo);
     }
@@ -9735,7 +9757,7 @@ void EmitBinaryReference(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator
     switch (pnode1->nop)
     {
     case knopName:
-        if (fLoadLhs && MayHaveSideEffectOnNode(pnode1, pnode2))
+        if (fLoadLhs && MayHaveSideEffectOnNode(pnode1, pnode2, byteCodeGenerator))
         {
             // Given x op y, y may kill x, so stash x.
             // Note that this only matters if we're loading x prior to the op.
@@ -9748,7 +9770,7 @@ void EmitBinaryReference(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator
             // We're loading the value of the LHS before the RHS, so make sure the LHS gets a register first.
             funcInfo->AcquireLoc(pnode1);
         }
-        if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2))
+        if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2, byteCodeGenerator))
         {
             // Given x.y op z, z may kill x, so stash x away.
             SaveOpndValue(pnode1->AsParseNodeBin()->pnode1, funcInfo);
@@ -9760,13 +9782,13 @@ void EmitBinaryReference(ParseNode *pnode1, ParseNode *pnode2, ByteCodeGenerator
             // We're loading the value of the LHS before the RHS, so make sure the LHS gets a register first.
             funcInfo->AcquireLoc(pnode1);
         }
-        if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2) ||
-            MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode1->AsParseNodeBin()->pnode2))
+        if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode2, byteCodeGenerator) ||
+            MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode1, pnode1->AsParseNodeBin()->pnode2, byteCodeGenerator))
         {
             // Given x[y] op z, y or z may kill x, so stash x away.
             SaveOpndValue(pnode1->AsParseNodeBin()->pnode1, funcInfo);
         }
-        if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode2, pnode2))
+        if (MayHaveSideEffectOnNode(pnode1->AsParseNodeBin()->pnode2, pnode2, byteCodeGenerator))
         {
             // Given x[y] op z, z may kill y, so stash y away.
             // But make sure that x gets a register before y.
@@ -9876,12 +9898,12 @@ bool CollectConcat(ParseNode *pnodeAdd, DListCounted<ParseNode *, ArenaAllocator
 void EmitConcat3(ParseNode *pnode, ParseNode *pnode1, ParseNode *pnode2, ParseNode *pnode3, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
 {
     byteCodeGenerator->StartStatement(pnode);
-    if (MayHaveSideEffectOnNode(pnode1, pnode2) || MayHaveSideEffectOnNode(pnode1, pnode3))
+    if (MayHaveSideEffectOnNode(pnode1, pnode2, byteCodeGenerator) || MayHaveSideEffectOnNode(pnode1, pnode3, byteCodeGenerator))
     {
         SaveOpndValue(pnode1, funcInfo);
     }
 
-    if (MayHaveSideEffectOnNode(pnode2, pnode3))
+    if (MayHaveSideEffectOnNode(pnode2, pnode3, byteCodeGenerator))
     {
         SaveOpndValue(pnode2, funcInfo);
     }

diff --git a/test/Miscellaneous/MayHaveSideEffectOnNodeSO.js b/test/Miscellaneous/MayHaveSideEffectOnNodeSO.js
@@ -0,0 +1,33 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// MayHaveSideEffectOnNode() was a recursive function that could recurse enough times to cause
+// an uncaught stack overflow.This was fixed by converting the recursive loop into an iterative
+// loop.
+//
+// An example of a program that caused the stack overflow is:
+//      eval("var a;a>(" + Array(2 ** 15).fill(0).join(",") + ");");
+//
+// MayHaveSideEffectOnNode() is originally called because the righthand side of the pNodeBin
+// ">" may overwrite the lefthand side of ">". The righthand side of pNodeBin is a deep tree
+// in which each pNode of the longest path is a pNodeBin(",").Since children of the original
+// pNodeBin -> right are pNodeBins themselves, MayHaveSideEffectOnNode() must check all of
+// their children as well.MayHaveSideEffectOnNode's original implementation was recursive and
+// thus the stack would overflow while recursing through the path of pNodeBins.   
+
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
+
+var tests = [
+    {
+        name: "MayHaveSideEffectOnNode() should not cause a stack overflow nor should fail to \
+terminate",
+        body: function () {
+            eval("var a;a>(" + Array(2 ** 15).fill(0).join(",") + ");");
+            eval("var a;a===(" + Array(2 ** 15).fill(0).join(",") + ");");
+        }
+    }
+]
+
+testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
diff --git a/test/Miscellaneous/rlexe.xml b/test/Miscellaneous/rlexe.xml
@@ -118,4 +118,10 @@
       <baseline>nullByte-string.baseline</baseline>
     </default>
   </test>
+  <test>
+    <default>
+      <files>MayHaveSideEffectOnNodeSO.js</files>
+      <compile-flags>-args summary -endargs</compile-flags>
+    </default>
+  </test>
 </regress-exe>