[array] Ensure PrepareElementsForSort returns a legal value

PrepareElementsForSort must return a number less than or equal the array
length.

Bug: chromium:897512, v8:7382
Change-Id: If5f9c4d052e623ab9f3300b8534603abbee859fa
Reviewed-on: https://chromium-review.googlesource.com/c/1297958
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56982}

src/runtime/runtime-array.cc

@@ -154,7 +154,15 @@ Object* RemoveArrayHolesGeneric(Isolate* isolate, Handle<JSReceiver> receiver,
     MAYBE_RETURN(delete_result, ReadOnlyRoots(isolate).exception());
   }
 
-  return *isolate->factory()->NewNumberFromUint(result);
+  // TODO(jgruber, szuend, chromium:897512): This is a workaround to prevent
+  // returning a number greater than array.length to Array.p.sort, which could
+  // trigger OOB accesses. There is still a correctness bug here though in
+  // how we shift around undefineds and delete elements in the two blocks above.
+  // This needs to be fixed soon.
+  const uint32_t number_of_non_undefined_elements = std::min(limit, result);
+
+  return *isolate->factory()->NewNumberFromUint(
+      number_of_non_undefined_elements);
 }
 
 // Collects all defined (non-hole) and non-undefined (array) elements at the
@@ -171,6 +179,7 @@ Object* RemoveArrayHoles(Isolate* isolate, Handle<JSReceiver> receiver,
   Handle<JSObject> object = Handle<JSObject>::cast(receiver);
   if (object->HasStringWrapperElements()) {
     int len = String::cast(Handle<JSValue>::cast(object)->value())->length();
+    DCHECK_LE(len, limit);
     return Smi::FromInt(len);
   }
 
@@ -293,6 +302,7 @@ Object* RemoveArrayHoles(Isolate* isolate, Handle<JSReceiver> receiver,
     }
   }
 
+  DCHECK_LE(result, limit);
   return *isolate->factory()->NewNumberFromUint(result);
 }
 

test/mjsunit/regress/regress-897512.js

+// 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.
+
+// Fill up the Array prototype's elements.
+for (let i = 0; i < 100; i++) Array.prototype.unshift(3.14);
+
+// Create a holey double elements array.
+const o31 = [1.1];
+o31[37] = 2.2;
+
+// Concat converts to dictionary elements.
+const o51 = o31.concat(false);
+
+// Set one element to undefined to trigger the movement bug.
+o51[0] = undefined;
+
+assertEquals(o51.length, 39);
+
+// Sort triggers the bug.
+o51.sort();
+
+// TODO(chromium:897512): The length should be 39.
+assertEquals(o51.length, 101);

third_party/v8/builtins/array-sort.tq

@@ -1716,7 +1716,6 @@ module array {
 
     // 2. Let obj be ? ToObject(this value).
     const obj: JSReceiver = ToObject(context, receiver);
-    let map: Map = obj.map;
 
     const sortState: FixedArray = AllocateZeroedFixedArray(kSortStateSize);
 
@@ -1724,22 +1723,24 @@ module array {
     sortState[kUserCmpFnIdx] = comparefnObj;
     sortState[kSortComparePtrIdx] =
         comparefnObj != Undefined ? SortCompareUserFn : SortCompareDefault;
-    sortState[kInitialReceiverMapIdx] = map;
     sortState[kBailoutStatusIdx] = kSuccess;
 
-    try {
-      const a: FastJSArray = Cast<FastJSArray>(receiver) otherwise Slow;
+    // 3. Let len be ? ToLength(? Get(obj, "length")).
+    const len: Number = GetLengthProperty(obj);
 
-      // 3. Let len be ? ToLength(? Get(obj, "length")).
-      const len: Smi = a.length;
-      if (len < 2) return receiver;
+    if (len < 2) return receiver;
 
-      // TODO(szuend): Investigate performance tradeoff of skipping this step
-      //               for PACKED_* and handling Undefineds during sorting.
-      const nofNonUndefined: Smi = PrepareElementsForSort(context, obj, len);
-      assert(a.map == map);
+    // TODO(szuend): Investigate performance tradeoff of skipping this step
+    //               for PACKED_* and handling Undefineds during sorting.
+    const nofNonUndefined: Smi = PrepareElementsForSort(context, obj, len);
+    assert(nofNonUndefined <= len);
 
-      sortState[kInitialReceiverLengthIdx] = len;
+    let map: Map = obj.map;
+    sortState[kInitialReceiverMapIdx] = map;
+    sortState[kInitialReceiverLengthIdx] = len;
+
+    try {
+      let a: FastJSArray = Cast<FastJSArray>(receiver) otherwise Slow;
 
       const elementsKind: ElementsKind = map.elements_kind;
       if (IsDoubleElementsKind(elementsKind)) {
@@ -1752,18 +1753,6 @@ module array {
       ArrayTimSort(context, sortState, nofNonUndefined);
     }
     label Slow {
-      // 3. Let len be ? ToLength(? Get(obj, "length")).
-      const len: Number = GetLengthProperty(obj);
-
-      if (len < 2) return receiver;
-      const nofNonUndefined: Smi = PrepareElementsForSort(context, obj, len);
-
-      sortState[kInitialReceiverLengthIdx] = len;
-
-      // Reload the map, PrepareElementsForSort might have changed the
-      // elements kind.
-      map = obj.map;
-
       if (map.elements_kind == DICTIONARY_ELEMENTS && IsExtensibleMap(map) &&
           !IsCustomElementsReceiverInstanceType(map.instance_type)) {
         InitializeSortStateAccessor<DictionaryElements>(sortState);