[builtins] Array.prototype.sort bug

Bug: chromium:743154
Change-Id: Id5b2a91a9242326b1dafccc4aeb95e18fb0fc8d8
Reviewed-on: https://chromium-review.googlesource.com/580928Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46873}

src/runtime/runtime-array.cc

@@ -51,8 +51,7 @@ namespace {
 // As PrepareElementsForSort, but only on objects where elements is
 // a dictionary, and it will stay a dictionary.  Collates undefined and
 // unexisting elements below limit from position zero of the elements.
-Handle<Object> PrepareSlowElementsForSort(Handle<JSObject> object,
-                                          uint32_t limit) {
+Object* PrepareSlowElementsForSort(Handle<JSObject> object, uint32_t limit) {
   DCHECK(object->HasDictionaryElements());
   Isolate* isolate = object->GetIsolate();
   // Must stay in dictionary mode, either because of requires_slow_elements,
@@ -66,10 +65,9 @@ Handle<Object> PrepareSlowElementsForSort(Handle<JSObject> object,
   uint32_t undefs = 0;
   uint32_t max_key = 0;
   int capacity = dict->Capacity();
-  Handle<Smi> bailout(Smi::FromInt(-1), isolate);
+  Smi* bailout = Smi::FromInt(-1);
   // Entry to the new dictionary does not cause it to grow, as we have
   // allocated one that is large enough for all entries.
-  DisallowHeapAllocation no_gc;
   for (int i = 0; i < capacity; i++) {
     Object* k;
     if (!dict->ToKey(isolate, i, &k)) continue;
@@ -90,24 +88,18 @@ Handle<Object> PrepareSlowElementsForSort(Handle<JSObject> object,
     if (key < limit) {
       if (value->IsUndefined(isolate)) {
         undefs++;
-      } else if (pos > static_cast<uint32_t>(Smi::kMaxValue)) {
-        // Adding an entry with the key beyond smi-range requires
-        // allocation. Bailout.
-        return bailout;
       } else {
         Handle<Object> result =
             SeededNumberDictionary::Add(new_dict, pos, value, details);
+        // Add should not grow the dictionary since we allocated the right size.
         DCHECK(result.is_identical_to(new_dict));
         USE(result);
         pos++;
       }
-    } else if (key > static_cast<uint32_t>(Smi::kMaxValue)) {
-      // Adding an entry with the key beyond smi-range requires
-      // allocation. Bailout.
-      return bailout;
     } else {
       Handle<Object> result =
           SeededNumberDictionary::Add(new_dict, key, value, details);
+      // Add should not grow the dictionary since we allocated the right size.
       DCHECK(result.is_identical_to(new_dict));
       USE(result);
       max_key = Max(max_key, key);
@@ -125,6 +117,7 @@ Handle<Object> PrepareSlowElementsForSort(Handle<JSObject> object,
     HandleScope scope(isolate);
     Handle<Object> result = SeededNumberDictionary::Add(
         new_dict, pos, isolate->factory()->undefined_value(), no_details);
+    // Add should not grow the dictionary since we allocated the right size.
     DCHECK(result.is_identical_to(new_dict));
     USE(result);
     pos++;
@@ -136,23 +129,21 @@ Handle<Object> PrepareSlowElementsForSort(Handle<JSObject> object,
   new_dict->UpdateMaxNumberKey(max_key, object);
   JSObject::ValidateElements(*object);
 
-  AllowHeapAllocation allocate_return_value;
-  return isolate->factory()->NewNumberFromUint(result);
+  return *isolate->factory()->NewNumberFromUint(result);
 }
 
 // Collects all defined (non-hole) and non-undefined (array) elements at the
 // start of the elements array.  If the object is in dictionary mode, it is
 // converted to fast elements mode.  Undefined values are placed after
 // non-undefined values.  Returns the number of non-undefined values.
-Handle<Object> PrepareElementsForSort(Handle<JSObject> object, uint32_t limit) {
+Object* PrepareElementsForSort(Handle<JSObject> object, uint32_t limit) {
   Isolate* isolate = object->GetIsolate();
   if (object->HasSloppyArgumentsElements() || !object->map()->is_extensible()) {
-    return handle(Smi::FromInt(-1), isolate);
+    return Smi::FromInt(-1);
   }
-
   if (object->HasStringWrapperElements()) {
     int len = String::cast(Handle<JSValue>::cast(object)->value())->length();
-    return handle(Smi::FromInt(len), isolate);
+    return Smi::FromInt(len);
   }
 
   JSObject::ValidateElements(*object);
@@ -178,9 +169,7 @@ Handle<Object> PrepareElementsForSort(Handle<JSObject> object, uint32_t limit) {
     JSObject::ValidateElements(*object);
   } else if (object->HasFixedTypedArrayElements()) {
     // Typed arrays cannot have holes or undefined elements.
-    return handle(
-        Smi::FromInt(FixedArrayBase::cast(object->elements())->length()),
-        isolate);
+    return Smi::FromInt(FixedArrayBase::cast(object->elements())->length());
   } else if (!object->HasDoubleElements()) {
     JSObject::EnsureWritableFastElements(object);
   }
@@ -195,7 +184,7 @@ Handle<Object> PrepareElementsForSort(Handle<JSObject> object, uint32_t limit) {
     limit = elements_length;
   }
   if (limit == 0) {
-    return handle(Smi::kZero, isolate);
+    return Smi::kZero;
   }
 
   uint32_t result = 0;
@@ -272,7 +261,7 @@ Handle<Object> PrepareElementsForSort(Handle<JSObject> object, uint32_t limit) {
     }
   }
 
-  return isolate->factory()->NewNumberFromUint(result);
+  return *isolate->factory()->NewNumberFromUint(result);
 }
 
 }  // namespace
@@ -289,7 +278,7 @@ RUNTIME_FUNCTION(Runtime_RemoveArrayHoles) {
   CONVERT_ARG_HANDLE_CHECKED(JSReceiver, object, 0);
   CONVERT_NUMBER_CHECKED(uint32_t, limit, Uint32, args[1]);
   if (object->IsJSProxy()) return Smi::FromInt(-1);
-  return *PrepareElementsForSort(Handle<JSObject>::cast(object), limit);
+  return PrepareElementsForSort(Handle<JSObject>::cast(object), limit);
 }
 
 

test/mjsunit/regress/regress-crbug-743154.js

+// Copyright 2017 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.
+
+
+
+Object.prototype[1] = 1.5;
+
+var v = { length: 12, [1073741824]: 0 };
+
+assertEquals(['1073741824', 'length'], Object.keys(v));
+assertEquals(undefined, v[0]);
+assertEquals(1.5, v[1]);
+assertEquals(0, v[1073741824]);
+
+// Properly handle out of range HeapNumber keys on 32bit platforms.
+Array.prototype.sort.call(v);
+
+assertEquals(['0', '1073741824', 'length'], Object.keys(v));
+assertEquals(1.5, v[0]);
+assertEquals(1.5, v[1]);
+assertEquals(0, v[1073741824]);