Fix broken bounds check in FTL's compileGetMyArgumentByVal().
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Feb 2018 07:30:30 +0000 (07:30 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Feb 2018 07:30:30 +0000 (07:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182419
<rdar://problem/37044945>

Reviewed by Saam Barati.

JSTests:

* stress/regress-182419.js: Added.

Source/JavaScriptCore:

In compileGetMyArgumentByVal(), it computes:
    limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
    ...
    LValue isOutOfBounds = m_out.aboveOrEqual(originalIndex, limit);

where the original "limit" is the number of arguments passed in by the caller.
If the original limit is less than numberOfArgumentsToSkip, the resultant limit
will be a large unsigned number.  As a result, this will defeat the bounds check
that follows it.

Note: later on in compileGetMyArgumentByVal(), we have to adjust adjust the index
value by adding numberOfArgumentsToSkip to it, in order to determine the actual
entry in the arguments array to get.

The fix is to just add numberOfArgumentsToSkip to index upfront (instead of
subtracting it from limit), and doing an overflow speculation check on that
addition before doing the bounds check.

* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileGetMyArgumentByVal):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227998 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/regress-182419.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 69b4196..472c750 100644 (file)
@@ -1,3 +1,13 @@
+2018-02-01  Mark Lam  <mark.lam@apple.com>
+
+        Fix broken bounds check in FTL's compileGetMyArgumentByVal().
+        https://bugs.webkit.org/show_bug.cgi?id=182419
+        <rdar://problem/37044945>
+
+        Reviewed by Saam Barati.
+
+        * stress/regress-182419.js: Added.
+
 2018-02-01  Keith Miller  <keith_miller@apple.com>
 
         Fix crashes due to mishandling custom sections.
diff --git a/JSTests/stress/regress-182419.js b/JSTests/stress/regress-182419.js
new file mode 100644 (file)
index 0000000..5b178dc
--- /dev/null
@@ -0,0 +1,28 @@
+function assertEqual(actual, expected) {
+    if (actual != expected)
+        throw "FAILED: expect " + expected + ", actual " + actual;
+}
+
+function test(index1, index2) {
+    function baz(a, b, c, ...args) {
+        return [args.length, args[index1], args[index2]];
+    }
+    function jaz(...args) {
+        return baz.apply(null, args);
+    }
+    noInline(jaz);
+
+    function check() {
+        let [length, a, b] = jaz();
+        assertEqual(length, 0);
+        assertEqual(a, undefined);
+        assertEqual(b, undefined);
+    }
+
+    for (let i = 0; i < 20000; i++) {
+        check();
+    }
+}
+
+test(0, 1);
+test(0x7fffffff, 0);
index 0daafdd..c695fbd 100644 (file)
@@ -1,3 +1,32 @@
+2018-02-01  Mark Lam  <mark.lam@apple.com>
+
+        Fix broken bounds check in FTL's compileGetMyArgumentByVal().
+        https://bugs.webkit.org/show_bug.cgi?id=182419
+        <rdar://problem/37044945>
+
+        Reviewed by Saam Barati.
+
+        In compileGetMyArgumentByVal(), it computes:
+            limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
+            ...
+            LValue isOutOfBounds = m_out.aboveOrEqual(originalIndex, limit);
+
+        where the original "limit" is the number of arguments passed in by the caller.
+        If the original limit is less than numberOfArgumentsToSkip, the resultant limit
+        will be a large unsigned number.  As a result, this will defeat the bounds check
+        that follows it.
+
+        Note: later on in compileGetMyArgumentByVal(), we have to adjust adjust the index
+        value by adding numberOfArgumentsToSkip to it, in order to determine the actual
+        entry in the arguments array to get.
+
+        The fix is to just add numberOfArgumentsToSkip to index upfront (instead of
+        subtracting it from limit), and doing an overflow speculation check on that
+        addition before doing the bounds check.
+
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileGetMyArgumentByVal):
+
 2018-02-01  Keith Miller  <keith_miller@apple.com>
 
         Fix crashes due to mishandling custom sections.
index 434a311..cdabee4 100644 (file)
@@ -4009,20 +4009,23 @@ private:
         
         LValue originalIndex = lowInt32(m_node->child2());
         
-        LValue originalLimit;
+        LValue numberOfArgsIncludingThis;
         if (inlineCallFrame && !inlineCallFrame->isVarargs())
-            originalLimit = m_out.constInt32(inlineCallFrame->argumentCountIncludingThis);
+            numberOfArgsIncludingThis = m_out.constInt32(inlineCallFrame->argumentCountIncludingThis);
         else {
             VirtualRegister argumentCountRegister = AssemblyHelpers::argumentCount(inlineCallFrame);
-            originalLimit = m_out.load32(payloadFor(argumentCountRegister));
+            numberOfArgsIncludingThis = m_out.load32(payloadFor(argumentCountRegister));
         }
         
-        LValue limit = m_out.sub(originalLimit, m_out.int32One);
-        
-        if (m_node->numberOfArgumentsToSkip())
-            limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
-        
-        LValue isOutOfBounds = m_out.aboveOrEqual(originalIndex, limit);
+        LValue numberOfArgs = m_out.sub(numberOfArgsIncludingThis, m_out.int32One);
+        LValue indexToCheck = originalIndex;
+        if (m_node->numberOfArgumentsToSkip()) {
+            CheckValue* check = m_out.speculateAdd(indexToCheck, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
+            blessSpeculation(check, Overflow, noValue(), nullptr, m_origin);
+            indexToCheck = check;
+        }
+
+        LValue isOutOfBounds = m_out.aboveOrEqual(indexToCheck, numberOfArgs);
         LBasicBlock continuation = nullptr;
         LBasicBlock lastNext = nullptr;
         ValueFromBlock slowResult;
@@ -4035,14 +4038,10 @@ private:
             
             lastNext = m_out.appendTo(normalCase, continuation);
         } else
-            speculate(OutOfBounds, noValue(), 0, isOutOfBounds);
-        
-        LValue index = originalIndex;
-        if (m_node->numberOfArgumentsToSkip())
-            index = m_out.add(index, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
-        
-        index = m_out.add(index, m_out.int32One);
+            speculate(OutOfBounds, noValue(), nullptr, isOutOfBounds);
         
+        LValue index = m_out.add(indexToCheck, m_out.int32One);
+
         TypedPointer base;
         if (inlineCallFrame) {
             if (inlineCallFrame->argumentCountIncludingThis > 1)
@@ -4055,7 +4054,7 @@ private:
             LValue pointer = m_out.baseIndex(
                 base.value(), m_out.zeroExt(index, pointerType()), ScaleEight);
             result = m_out.load64(TypedPointer(m_heaps.variables.atAnyIndex(), pointer));
-            result = preciseIndexMask32(result, originalIndex, limit);
+            result = preciseIndexMask32(result, indexToCheck, numberOfArgs);
         } else
             result = m_out.constInt64(JSValue::encode(jsUndefined()));