Changeset 227998 in webkit

Timestamp:

Feb 1, 2018 11:30:30 PM (6 years ago)

Author:

mark.lam@apple.com

Message:

Fix broken bounds check in FTL's compileGetMyArgumentByVal().
​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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/regress-182419.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -4010 +4010 @@
         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));
-        }
-        
-        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);
+            numberOfArgsIncludingThis = m_out.load32(payloadFor(argumentCountRegister));
+        }
+        
+        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;
@@ -4036 +4039 @@
             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) {
@@ -4056 +4055 @@
                 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()));