Argument elimination should check for negative indices in GetByVal
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Jun 2019 17:56:59 +0000 (17:56 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Jun 2019 17:56:59 +0000 (17:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198302
<rdar://problem/51188095>

Reviewed by Filip Pizlo.

JSTests:

* stress/eliminate-arguments-negative-rest-access.js: Added.
(inlinee):
(opt):

Source/JavaScriptCore:

In DFG::ArgumentEliminationPhase, the index is treated as unsigned, but there's no check
for overflow in the addition. In compileGetMyArgumentByVal, there's a check for overflow,
but the index is treated as signed, resulting in an index lower than numberOfArgumentsToSkip.

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

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

JSTests/ChangeLog
JSTests/stress/eliminate-arguments-negative-rest-access.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index bb67886..c42825b 100644 (file)
@@ -1,3 +1,15 @@
+2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
+
+        Argument elimination should check for negative indices in GetByVal
+        https://bugs.webkit.org/show_bug.cgi?id=198302
+        <rdar://problem/51188095>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/eliminate-arguments-negative-rest-access.js: Added.
+        (inlinee):
+        (opt):
+
 2019-06-03  Caio Lima  <ticaiolima@gmail.com>
 
         [ESNext][BigInt] Implement support for "**"
diff --git a/JSTests/stress/eliminate-arguments-negative-rest-access.js b/JSTests/stress/eliminate-arguments-negative-rest-access.js
new file mode 100644 (file)
index 0000000..11c55ff
--- /dev/null
@@ -0,0 +1,16 @@
+//@ requireOptions("--forceEagerCompilation=1")
+
+function inlinee(index, value, ...rest) {
+    return rest[index | 0];
+}
+
+function opt() {
+    return inlinee(-1, 0x1234);
+}
+noInline(opt);
+
+for (let i = 0; i < 1e6; i++) {
+    const value = opt();
+    if (value !== undefined)
+        throw new Error(`${i}: ${value}`);
+}
index 369173f..40a5487 100644 (file)
@@ -1,5 +1,21 @@
 2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
 
+        Argument elimination should check for negative indices in GetByVal
+        https://bugs.webkit.org/show_bug.cgi?id=198302
+        <rdar://problem/51188095>
+
+        Reviewed by Filip Pizlo.
+
+        In DFG::ArgumentEliminationPhase, the index is treated as unsigned, but there's no check
+        for overflow in the addition. In compileGetMyArgumentByVal, there's a check for overflow,
+        but the index is treated as signed, resulting in an index lower than numberOfArgumentsToSkip.
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileGetMyArgumentByVal):
+
+2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
+
         JSScript should not keep bytecode cache in memory
         https://bugs.webkit.org/show_bug.cgi?id=198482
 
index 2b7a7c6..b7e4a4f 100644 (file)
@@ -762,11 +762,11 @@ private:
                         InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame();
                         index += numberOfArgumentsToSkip;
                         
-                        bool safeToGetStack;
+                        bool safeToGetStack = index >= numberOfArgumentsToSkip;
                         if (inlineCallFrame)
-                            safeToGetStack = index < inlineCallFrame->argumentCountIncludingThis - 1;
+                            safeToGetStack &= index < inlineCallFrame->argumentCountIncludingThis - 1;
                         else {
-                            safeToGetStack =
+                            safeToGetStack &=
                                 index < static_cast<unsigned>(codeBlock()->numParameters()) - 1;
                         }
                         if (safeToGetStack) {
index 94c9658..873ae3b 100644 (file)
@@ -4461,13 +4461,15 @@ private:
         
         LValue numberOfArgs = m_out.sub(numberOfArgsIncludingThis, m_out.int32One);
         LValue indexToCheck = originalIndex;
+        LValue numberOfArgumentsToSkip = m_out.int32Zero;
         if (m_node->numberOfArgumentsToSkip()) {
-            CheckValue* check = m_out.speculateAdd(indexToCheck, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
+            numberOfArgumentsToSkip = m_out.constInt32(m_node->numberOfArgumentsToSkip());
+            CheckValue* check = m_out.speculateAdd(indexToCheck, numberOfArgumentsToSkip);
             blessSpeculation(check, Overflow, noValue(), nullptr, m_origin);
             indexToCheck = check;
         }
 
-        LValue isOutOfBounds = m_out.aboveOrEqual(indexToCheck, numberOfArgs);
+        LValue isOutOfBounds = m_out.bitOr(m_out.aboveOrEqual(indexToCheck, numberOfArgs), m_out.below(indexToCheck, numberOfArgumentsToSkip));
         LBasicBlock continuation = nullptr;
         LBasicBlock lastNext = nullptr;
         ValueFromBlock slowResult;