Array.prototype.indexOf fast path needs to ensure the length is still valid after...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Sep 2018 23:05:54 +0000 (23:05 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Sep 2018 23:05:54 +0000 (23:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189922
<rdar://problem/44651275>

Reviewed by Mark Lam.

JSTests:

* stress/array-indexof-fast-path-effects.js: Added.
* stress/array-indexof-cached-length.js: Added.

Source/JavaScriptCore:

The implementation was first getting the length to iterate up to,
then getting the starting index. However, getting the starting
index may perform effects. e.g, it could change the length of the
array. This changes it so we verify the length is still valid.

* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncIndexOf):

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

JSTests/ChangeLog
JSTests/stress/array-indexof-cached-length.js [new file with mode: 0644]
JSTests/stress/array-indexof-fast-path-effects.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ArrayPrototype.cpp

index 06b9ff8..95ce2ed 100644 (file)
@@ -1,3 +1,14 @@
+2018-09-24  Saam Barati  <sbarati@apple.com>
+
+        Array.prototype.indexOf fast path needs to ensure the length is still valid after performing effects
+        https://bugs.webkit.org/show_bug.cgi?id=189922
+        <rdar://problem/44651275>
+
+        Reviewed by Mark Lam.
+
+        * stress/array-indexof-fast-path-effects.js: Added.
+        * stress/array-indexof-cached-length.js: Added.
+
 2018-09-24  Saam barati  <sbarati@apple.com>
 
         ArgumentsEliminationPhase should snip basic blocks after proven OSR exits
diff --git a/JSTests/stress/array-indexof-cached-length.js b/JSTests/stress/array-indexof-cached-length.js
new file mode 100644 (file)
index 0000000..602f8a2
--- /dev/null
@@ -0,0 +1,24 @@
+function assert(b) {
+    if (!b)
+        throw new Error;
+
+}
+
+const originalLength = 10000;
+let arr = new Proxy([], {
+    has(...args) {
+        assert(parseInt(args[1]) < originalLength);
+        assert(args[0].length - 10 === originalLength);
+        return Reflect.has(...args);
+    }
+});
+
+for (var i = 0; i < originalLength; i++)
+    arr[i] = [];
+
+arr.indexOf(new Object(), {
+    valueOf: function () {
+        arr.length += 10;
+        return 0;
+    }
+});
diff --git a/JSTests/stress/array-indexof-fast-path-effects.js b/JSTests/stress/array-indexof-fast-path-effects.js
new file mode 100644 (file)
index 0000000..2dac83c
--- /dev/null
@@ -0,0 +1,11 @@
+// This shouldn't crash when running with ASAN.
+let arr = [];
+for (var i = 0; i < 1000000; i++)
+    arr[i] = [];
+
+arr.indexOf(new Object(), {
+    valueOf: function () {
+        arr.length = 0;
+        return 0;
+    }
+});
index a5eccf3..7d33a4c 100644 (file)
@@ -1,3 +1,19 @@
+2018-09-24  Saam Barati  <sbarati@apple.com>
+
+        Array.prototype.indexOf fast path needs to ensure the length is still valid after performing effects
+        https://bugs.webkit.org/show_bug.cgi?id=189922
+        <rdar://problem/44651275>
+
+        Reviewed by Mark Lam.
+
+        The implementation was first getting the length to iterate up to,
+        then getting the starting index. However, getting the starting
+        index may perform effects. e.g, it could change the length of the
+        array. This changes it so we verify the length is still valid.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncIndexOf):
+
 2018-09-24  Tadeu Zagallo  <tzagallo@apple.com>
 
         offlineasm: fix macro scoping
index f714c1f..31a03a7 100644 (file)
@@ -1169,7 +1169,9 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncIndexOf(ExecState* exec)
 
     if (isJSArray(thisObject)) {
         JSArray* array = asArray(thisObject);
-        if (array->canDoFastIndexedAccess(vm)) {
+        bool canDoFastPath = array->canDoFastIndexedAccess(vm)
+            && array->getArrayLength() == length; // The effects in getting `index` could have changed the length of this array.
+        if (canDoFastPath) {
             switch (array->indexingType()) {
             case ALL_INT32_INDEXING_TYPES: {
                 if (!searchElement.isNumber())