We should not crash there is a finally inside a for-in loop

https://bugs.webkit.org/show_bug.cgi?id=159243 <rdar://problem/27018910> Reviewed by Benjamin Poulain. Previously we would swap the m_forInContext with an empty vector then attempt to shrink the size of m_forInContext by the amount we expected. This meant that if there was more than one ForInContext on the stack and we wanted to pop exactly one off we would crash. This patch makes ForInContexts RefCounted so they can be duplicated into other vectors. It also has ForInContexts copy the entire stack rather than do the swap that we did before. This makes ForInContexts work the same as the other contexts. * bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::emitComplexPopScopes): (JSC::BytecodeGenerator::pushIndexedForInScope): (JSC::BytecodeGenerator::pushStructureForInScope): * bytecompiler/BytecodeGenerator.h: * tests/stress/finally-for-in.js: Added. (repeat): (createSimple): Canonical link: https://commits.webkit.org/177355@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@202608 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 29, 2016 · d996900 · d996900
1 parent a3d46d8
commit d996900
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 7 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,29 @@
+2016-06-28  Keith Miller  <keith_miller@apple.com>
+
+        We should not crash there is a finally inside a for-in loop
+        https://bugs.webkit.org/show_bug.cgi?id=159243
+        <rdar://problem/27018910>
+
+        Reviewed by Benjamin Poulain.
+
+        Previously we would swap the m_forInContext with an empty vector
+        then attempt to shrink the size of m_forInContext by the amount
+        we expected. This meant that if there was more than one ForInContext
+        on the stack and we wanted to pop exactly one off we would crash.
+        This patch makes ForInContexts RefCounted so they can be duplicated
+        into other vectors. It also has ForInContexts copy the entire stack
+        rather than do the swap that we did before. This makes ForInContexts
+        work the same as the other contexts.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitComplexPopScopes):
+        (JSC::BytecodeGenerator::pushIndexedForInScope):
+        (JSC::BytecodeGenerator::pushStructureForInScope):
+        * bytecompiler/BytecodeGenerator.h:
+        * tests/stress/finally-for-in.js: Added.
+        (repeat):
+        (createSimple):
+
 2016-06-28  Saam Barati  <sbarati@apple.com>
 
         Assertion failure or crash when accessing let-variable in TDZ with eval with a function in it that returns let variable

diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
@@ -3562,7 +3562,7 @@ void BytecodeGenerator::emitComplexPopScopes(RegisterID* scope, ControlFlowConte
 
         Vector<ControlFlowContext> savedScopeContextStack;
         Vector<SwitchInfo> savedSwitchContextStack;
-        Vector<std::unique_ptr<ForInContext>> savedForInContextStack;
+        Vector<RefPtr<ForInContext>> savedForInContextStack;
         Vector<TryContext> poppedTryContexts;
         Vector<SymbolTableStackEntry> savedSymbolTableStack;
         LabelScopeStore savedLabelScopes;
@@ -3591,7 +3591,7 @@ void BytecodeGenerator::emitComplexPopScopes(RegisterID* scope, ControlFlowConte
                 m_switchContextStack.shrink(finallyContext.switchContextStackSize);
             }
             if (flipForIns) {
-                savedForInContextStack.swap(m_forInContextStack);
+                savedForInContextStack = m_forInContextStack;
                 m_forInContextStack.shrink(finallyContext.forInContextStackSize);
             }
             if (flipTries) {
@@ -3641,7 +3641,7 @@ void BytecodeGenerator::emitComplexPopScopes(RegisterID* scope, ControlFlowConte
             if (flipSwitches)
                 m_switchContextStack = savedSwitchContextStack;
             if (flipForIns)
-                m_forInContextStack.swap(savedForInContextStack);
+                m_forInContextStack = savedForInContextStack;
             if (flipTries) {
                 ASSERT(m_tryContextStack.size() == finallyContext.tryContextStackSize);
                 for (unsigned i = poppedTryContexts.size(); i--;) {
@@ -4211,7 +4211,7 @@ void BytecodeGenerator::pushIndexedForInScope(RegisterID* localRegister, Registe
 {
     if (!localRegister)
         return;
-    m_forInContextStack.append(std::make_unique<IndexedForInContext>(localRegister, indexRegister));
+    m_forInContextStack.append(adoptRef(new IndexedForInContext(localRegister, indexRegister)));
 }
 
 void BytecodeGenerator::popIndexedForInScope(RegisterID* localRegister)
@@ -4321,7 +4321,7 @@ void BytecodeGenerator::pushStructureForInScope(RegisterID* localRegister, Regis
 {
     if (!localRegister)
         return;
-    m_forInContextStack.append(std::make_unique<StructureForInContext>(localRegister, indexRegister, propertyRegister, enumeratorRegister));
+    m_forInContextStack.append(adoptRef(new StructureForInContext(localRegister, indexRegister, propertyRegister, enumeratorRegister)));
 }
 
 void BytecodeGenerator::popStructureForInScope(RegisterID* localRegister)

diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
@@ -102,8 +102,9 @@ namespace JSC {
         FinallyContext finallyContext;
     };
 
-    class ForInContext {
+    class ForInContext : public RefCounted<ForInContext> {
         WTF_MAKE_FAST_ALLOCATED;
+        WTF_MAKE_NONCOPYABLE(ForInContext);
     public:
         ForInContext(RegisterID* localRegister)
             : m_localRegister(localRegister)
@@ -919,7 +920,7 @@ namespace JSC {
 
         Vector<ControlFlowContext, 0, UnsafeVectorOverflow> m_scopeContextStack;
         Vector<SwitchInfo> m_switchContextStack;
-        Vector<std::unique_ptr<ForInContext>> m_forInContextStack;
+        Vector<RefPtr<ForInContext>> m_forInContextStack;
         Vector<TryContext> m_tryContextStack;
         Vector<RefPtr<Label>> m_generatorResumeLabels;
         enum FunctionVariableType : uint8_t { NormalFunctionVariable, GlobalFunctionVariable };

diff --git a/Source/JavaScriptCore/tests/stress/finally-for-in.js b/Source/JavaScriptCore/tests/stress/finally-for-in.js
@@ -0,0 +1,38 @@
+function repeat(count, thunk) {
+    let result = "";
+    for (let i = 0; i < count; i++)
+        result += thunk(i);
+    return result;
+}
+
+function createSimple(outerDepth, innerDepth, returnDepth) {
+    return Function(
+        `
+        return (function(arg) {
+            ${repeat(outerDepth, (i) => `for (let a${i} in arg) ` + "{\n" )}
+            try {
+                ${repeat(innerDepth, (i) => `for (let b${i} in arg) ` + "{\n" )}
+                return {};
+                ${repeat(innerDepth, () => "}")}
+            }
+            finally { return a${returnDepth}}
+            ${repeat(outerDepth, () => "}")}
+        })
+        `
+    )();
+}
+
+function test(result, argument, ...args) {
+    let f = createSimple(...args);
+
+    let r = f(argument);
+    if (r !== result) {
+        throw new Error(r);
+    }
+}
+
+
+test("0", [1,2], 1, 1, 0);
+test("0", [1,2], 2, 1, 0);
+test("0", [1,2], 2, 4, 1);
+test("0", [1,2], 1, 0, 0);