JavaScript rest function parameter with negative index leads to bad DFG abstract...

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

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index b88fce6..a29dbae 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,17 @@
+2017-11-27  JF Bastien  <jfbastien@apple.com>
+
+        JavaScript rest function parameter with negative index leads to bad DFG abstract interpretation
+        https://bugs.webkit.org/show_bug.cgi?id=180051
+        <rdar://problem/35614371>
+
+        Reviewed by Saam Barati.
+
+        * stress/rest-parameter-negative.js: Added.
+        (__f_5484):
+        (catch):
+        (__f_5485):
+        (__v_22598.catch):
+
 2017-11-27  Saam Barati  <sbarati@apple.com>
 
         Spread can escape when CreateRest does not

diff --git a/JSTests/stress/rest-parameter-negative.js b/JSTests/stress/rest-parameter-negative.js
new file mode 100644 (file)
index 0000000..632dad9
--- /dev/null
+++ b/JSTests/stress/rest-parameter-negative.js
@@ -0,0 +1,21 @@
+function __f_5484(__v_22596) {
+  if (!__v_22596) throw new Error();
+}
+
+try {
+  noInline(__f_5484);
+} catch (e) {}
+
+function __f_5485(...__v_22597) {
+  return __v_22597[-13];
+}
+
+try {
+  noInline(__f_5485);
+} catch (e) {}
+
+for (let __v_22598 = 0; __v_22598 < 10000; __v_22598++) {
+  try {
+    __f_5484(__f_5485(__v_22598) === __v_22598);
+  } catch (e) {}
+}

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 840e181..63c27ac 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,17 @@
+2017-11-27  JF Bastien  <jfbastien@apple.com>
+
+        JavaScript rest function parameter with negative index leads to bad DFG abstract interpretation
+        https://bugs.webkit.org/show_bug.cgi?id=180051
+        <rdar://problem/35614371>
+
+        Reviewed by Saam Barati.
+
+        Checking for int32 isn't sufficient when uint32 is expected
+        afterwards. While we're here, also use Checked<>.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+
 2017-11-14  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Move JSONValues to WTF and convert uses of InspectorValues.h to JSONValues.h

diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
index 59071df..8b78e00 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
@@ -40,6 +40,8 @@
 #include "PutByIdStatus.h"
 #include "StringObject.h"
 
+#include <wtf/CheckedArithmetic.h>
+
 namespace JSC { namespace DFG {
 
 template<typename AbstractStateType>
@@ -1877,24 +1879,28 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         JSValue index = forNode(node->child2()).m_value;
         InlineCallFrame* inlineCallFrame = node->child1()->origin.semantic.inlineCallFrame;
 
-        if (index && index.isInt32()) {
+        if (index && index.isUInt32()) {
             // This pretends to return TOP for accesses that are actually proven out-of-bounds because
             // that's the conservative thing to do. Otherwise we'd need to write more code to mark such
             // paths as unreachable, or to return undefined. We could implement that eventually.
-            
-            unsigned argumentIndex = index.asUInt32() + node->numberOfArgumentsToSkip();
-            if (inlineCallFrame) {
-                if (argumentIndex < inlineCallFrame->argumentCountIncludingThis - 1) {
-                    forNode(node) = m_state.variables().operand(
-                        virtualRegisterForArgument(argumentIndex + 1) + inlineCallFrame->stackOffset);
-                    m_state.setFoundConstants(true);
-                    break;
-                }
-            } else {
-                if (argumentIndex < m_state.variables().numberOfArguments() - 1) {
-                    forNode(node) = m_state.variables().argument(argumentIndex + 1);
-                    m_state.setFoundConstants(true);
-                    break;
+
+            Checked<unsigned, RecordOverflow> argumentIndexChecked = index.asUInt32();
+            argumentIndexChecked += node->numberOfArgumentsToSkip();
+            unsigned argumentIndex;
+            if (argumentIndexChecked.safeGet(argumentIndex) != CheckedState::DidOverflow) {
+                if (inlineCallFrame) {
+                    if (argumentIndex < inlineCallFrame->argumentCountIncludingThis - 1) {
+                        forNode(node) = m_state.variables().operand(
+                            virtualRegisterForArgument(argumentIndex + 1) + inlineCallFrame->stackOffset);
+                        m_state.setFoundConstants(true);
+                        break;
+                    }
+                } else {
+                    if (argumentIndex < m_state.variables().numberOfArguments() - 1) {
+                        forNode(node) = m_state.variables().argument(argumentIndex + 1);
+                        m_state.setFoundConstants(true);
+                        break;
+                    }
                 }
             }
         }