[builtins] Fix out-of-bounds in Array#lastIndexOf().

bmeurer · Commit Bot · commit b8a911314d00 · 2018-11-02T07:42:50.000Z
The fast-path in the `ArrayPrototypeLastIndexOf` torque implementation didn't check that the `fromIndex` is within the bounds of the JSArray _AFTER_ the call to ToInteger, which can have arbitrary side-effects, i.e. it can change the length of the array. R=yangguo@chromium.org Bug: chromium:898785 Change-Id: I7ef84143ec8c33148f6e9d451bd52769d5074fb4 Reviewed-on: https://chromium-review.googlesource.com/c/1314329 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Cr-Commit-Position: refs/heads/master@{#57204}
diff --git a/src/builtins/array-lastindexof.tq b/src/builtins/array-lastindexof.tq
@@ -25,10 +25,18 @@ module array {
   }
 
   macro FastArrayLastIndexOf<Elements: type>(
-      context: Context, array: JSArray, length: Smi, from: Smi,
-      searchElement: Object): Smi {
+      context: Context, array: JSArray, from: Smi, searchElement: Object): Smi {
     const elements: FixedArrayBase = array.elements;
     let k: Smi = from;
+
+    // Bug(898785): Due to side-effects in the evaluation of `fromIndex`
+    // the {from} can be out-of-bounds here, so we need to clamp {k} to
+    // the {elements} length. We might be reading holes / hole NaNs still
+    // due to that, but those will be ignored below.
+    if (k >= elements.length) {
+      k = elements.length - 1;
+    }
+
     while (k >= 0) {
       try {
         const element: Object = LoadWithHoleCheck<Elements>(elements, k)
@@ -83,11 +91,11 @@ module array {
     const kind: ElementsKind = array.map.elements_kind;
     if (IsFastSmiOrTaggedElementsKind(kind)) {
       return FastArrayLastIndexOf<FixedArray>(
-          context, array, length, fromSmi, searchElement);
+          context, array, fromSmi, searchElement);
     }
     assert(IsDoubleElementsKind(kind));
     return FastArrayLastIndexOf<FixedDoubleArray>(
-        context, array, length, fromSmi, searchElement);
+        context, array, fromSmi, searchElement);
   }
 
   transitioning macro GenericArrayLastIndexOf(
diff --git a/test/mjsunit/regress/regress-crbug-898785.js b/test/mjsunit/regress/regress-crbug-898785.js
@@ -0,0 +1,11 @@
+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+var a = [0, 1];
+var o = { [Symbol.toPrimitive]() { a.length = 1; return 2; } };
+
+a.push(2);
+a.lastIndexOf(5, o);
