[runtime] Throw range error on too many properties

This change allows the KeyAccumulator to throw a range error if there
are too many properties to be enumerated.

This CL introduces extensive checks during key enumeration in the run-time,
and might introduce regressions. If so, feel free to revert.

Bug: chromium:918301
Change-Id: I6166c0b15f1a05eac7116a979f12ba4833d1d1b1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1545902
Auto-Submit: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63430}

src/common/globals.h

@@ -1583,6 +1583,11 @@ constexpr int kSmallOrderedHashMapMinCapacity = 4;
 // has correct value range (see Issue 830 for more details).
 enum StackFrameId { ID_MIN_VALUE = kMinInt, ID_MAX_VALUE = kMaxInt, NO_ID = 0 };
 
+enum class ExceptionStatus : bool { kException = false, kSuccess = true };
+V8_INLINE bool operator!(ExceptionStatus status) {
+  return !static_cast<bool>(status);
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/common/message-template.h

@@ -485,6 +485,7 @@ namespace internal {
     "Too many arguments in function call (only 65535 allowed)")                \
   T(TooManyParameters,                                                         \
     "Too many parameters in function definition (only 65534 allowed)")         \
+  T(TooManyProperties, "Too many properties to enumerate")                     \
   T(TooManySpreads,                                                            \
     "Literal containing too many nested spreads (up to 65534 allowed)")        \
   T(TooManyVariables, "Too many variables declared (only 4194303 allowed)")    \

src/heap/factory.cc

@@ -686,16 +686,19 @@ Handle<SmallOrderedNameDictionary> Factory::NewSmallOrderedNameDictionary(
 }
 
 Handle<OrderedHashSet> Factory::NewOrderedHashSet() {
-  return OrderedHashSet::Allocate(isolate(), OrderedHashSet::kMinCapacity);
+  return OrderedHashSet::Allocate(isolate(), OrderedHashSet::kMinCapacity)
+      .ToHandleChecked();
 }
 
 Handle<OrderedHashMap> Factory::NewOrderedHashMap() {
-  return OrderedHashMap::Allocate(isolate(), OrderedHashMap::kMinCapacity);
+  return OrderedHashMap::Allocate(isolate(), OrderedHashMap::kMinCapacity)
+      .ToHandleChecked();
 }
 
 Handle<OrderedNameDictionary> Factory::NewOrderedNameDictionary() {
   return OrderedNameDictionary::Allocate(isolate(),
-                                         OrderedNameDictionary::kMinCapacity);
+                                         OrderedNameDictionary::kMinCapacity)
+      .ToHandleChecked();
 }
 
 Handle<AccessorPair> Factory::NewAccessorPair() {

src/heap/heap.cc

@@ -5921,7 +5921,7 @@ void Heap::KeepDuringJob(Handle<JSReceiver> target) {
     table =
         handle(OrderedHashSet::cast(weak_refs_keep_during_job()), isolate());
   }
-  table = OrderedHashSet::Add(isolate(), table, target);
+  table = OrderedHashSet::Add(isolate(), table, target).ToHandleChecked();
   set_weak_refs_keep_during_job(*table);
 }
 

src/json/json-stringifier.cc

@@ -269,7 +269,11 @@ bool JsonStringifier::InitializeReplacer(Handle<Object> replacer) {
       if (key.is_null()) continue;
       // Object keys are internalized, so do it here.
       key = factory()->InternalizeString(key);
-      set = OrderedHashSet::Add(isolate_, set, key);
+      MaybeHandle<OrderedHashSet> set_candidate =
+          OrderedHashSet::Add(isolate_, set, key);
+      if (!set_candidate.ToHandle(&set)) {
+        return false;
+      }
     }
     property_list_ = OrderedHashSet::ConvertToKeysArray(
         isolate_, set, GetKeysConversion::kKeepNumbers);

src/objects/dictionary.h

@@ -170,7 +170,8 @@ class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE) BaseNameDictionary
 
   // Collect the keys into the given KeyAccumulator, in ascending chronological
   // order of property creation.
-  static void CollectKeysTo(Handle<Derived> dictionary, KeyAccumulator* keys);
+  V8_WARN_UNUSED_RESULT static ExceptionStatus CollectKeysTo(
+      Handle<Derived> dictionary, KeyAccumulator* keys);
 
   // Return the key indices sorted by its enumeration index.
   static Handle<FixedArray> IterationIndices(Isolate* isolate,

src/objects/elements-inl.h

@@ -5,6 +5,7 @@
 #ifndef V8_OBJECTS_ELEMENTS_INL_H_
 #define V8_OBJECTS_ELEMENTS_INL_H_
 
+#include "src/common/globals.h"
 #include "src/objects/elements.h"
 
 #include "src/handles/handles-inl.h"
@@ -13,10 +14,11 @@
 namespace v8 {
 namespace internal {
 
-inline void ElementsAccessor::CollectElementIndices(Handle<JSObject> object,
-                                                    KeyAccumulator* keys) {
-  CollectElementIndices(object, handle(object->elements(), keys->isolate()),
-                        keys);
+V8_WARN_UNUSED_RESULT inline ExceptionStatus
+ElementsAccessor::CollectElementIndices(Handle<JSObject> object,
+                                        KeyAccumulator* keys) {
+  return CollectElementIndices(
+      object, handle(object->elements(), keys->isolate()), keys);
 }
 
 inline MaybeHandle<FixedArray> ElementsAccessor::PrependElementIndices(

src/objects/elements.h

@@ -69,12 +69,12 @@ class ElementsAccessor {
   // Copy all indices that have elements from |object| into the given
   // KeyAccumulator. For Dictionary-based element-kinds we filter out elements
   // whose PropertyAttribute match |filter|.
-  virtual void CollectElementIndices(Handle<JSObject> object,
-                                     Handle<FixedArrayBase> backing_store,
-                                     KeyAccumulator* keys) = 0;
+  V8_WARN_UNUSED_RESULT virtual ExceptionStatus CollectElementIndices(
+      Handle<JSObject> object, Handle<FixedArrayBase> backing_store,
+      KeyAccumulator* keys) = 0;
 
-  inline void CollectElementIndices(Handle<JSObject> object,
-                                    KeyAccumulator* keys);
+  V8_WARN_UNUSED_RESULT inline ExceptionStatus CollectElementIndices(
+      Handle<JSObject> object, KeyAccumulator* keys);
 
   virtual Maybe<bool> CollectValuesOrEntries(
       Isolate* isolate, Handle<JSObject> object,
@@ -90,9 +90,9 @@ class ElementsAccessor {
       Handle<JSObject> object, Handle<FixedArray> keys,
       GetKeysConversion convert, PropertyFilter filter = ALL_PROPERTIES);
 
-  virtual void AddElementsToKeyAccumulator(Handle<JSObject> receiver,
-                                           KeyAccumulator* accumulator,
-                                           AddKeyConversion convert) = 0;
+  V8_WARN_UNUSED_RESULT virtual ExceptionStatus AddElementsToKeyAccumulator(
+      Handle<JSObject> receiver, KeyAccumulator* accumulator,
+      AddKeyConversion convert) = 0;
 
   virtual void TransitionElementsKind(Handle<JSObject> object,
                                       Handle<Map> map) = 0;

src/objects/keys.h

@@ -52,8 +52,8 @@ class KeyAccumulator final {
                                        Handle<JSObject> object);
   Maybe<bool> CollectOwnPropertyNames(Handle<JSReceiver> receiver,
                                       Handle<JSObject> object);
-  void CollectPrivateNames(Handle<JSReceiver> receiver,
-                           Handle<JSObject> object);
+  V8_WARN_UNUSED_RESULT ExceptionStatus
+  CollectPrivateNames(Handle<JSReceiver> receiver, Handle<JSObject> object);
   Maybe<bool> CollectAccessCheckInterceptorKeys(
       Handle<AccessCheckInfo> access_check_info, Handle<JSReceiver> receiver,
       Handle<JSObject> object);
@@ -65,10 +65,14 @@ class KeyAccumulator final {
   static Handle<FixedArray> GetOwnEnumPropertyKeys(Isolate* isolate,
                                                    Handle<JSObject> object);
 
-  void AddKey(Object key, AddKeyConversion convert = DO_NOT_CONVERT);
-  void AddKey(Handle<Object> key, AddKeyConversion convert = DO_NOT_CONVERT);
-  void AddKeys(Handle<FixedArray> array, AddKeyConversion convert);
-  void AddKeys(Handle<JSObject> array_like, AddKeyConversion convert);
+  V8_WARN_UNUSED_RESULT ExceptionStatus
+  AddKey(Object key, AddKeyConversion convert = DO_NOT_CONVERT);
+  V8_WARN_UNUSED_RESULT ExceptionStatus
+  AddKey(Handle<Object> key, AddKeyConversion convert = DO_NOT_CONVERT);
+  V8_WARN_UNUSED_RESULT ExceptionStatus AddKeys(Handle<FixedArray> array,
+                                                AddKeyConversion convert);
+  V8_WARN_UNUSED_RESULT ExceptionStatus AddKeys(Handle<JSObject> array_like,
+                                                AddKeyConversion convert);
 
   // Jump to the next level, pushing the current |levelLength_| to
   // |levelLengths_| and adding a new list to |elements_|.

src/objects/objects.cc

@@ -7430,7 +7430,7 @@ Handle<FixedArray> BaseNameDictionary<Derived, Shape>::IterationIndices(
 }
 
 template <typename Derived, typename Shape>
-void BaseNameDictionary<Derived, Shape>::CollectKeysTo(
+ExceptionStatus BaseNameDictionary<Derived, Shape>::CollectKeysTo(
     Handle<Derived> dictionary, KeyAccumulator* keys) {
   Isolate* isolate = keys->isolate();
   ReadOnlyRoots roots(isolate);
@@ -7475,16 +7475,19 @@ void BaseNameDictionary<Derived, Shape>::CollectKeysTo(
       has_seen_symbol = true;
       continue;
     }
-    keys->AddKey(key, DO_NOT_CONVERT);
+    ExceptionStatus status = keys->AddKey(key, DO_NOT_CONVERT);
+    if (!status) return status;
   }
   if (has_seen_symbol) {
     for (int i = 0; i < array_size; i++) {
       int index = Smi::ToInt(array->get(i));
       Object key = dictionary->NameAt(index);
       if (!key.IsSymbol()) continue;
-      keys->AddKey(key, DO_NOT_CONVERT);
+      ExceptionStatus status = keys->AddKey(key, DO_NOT_CONVERT);
+      if (!status) return status;
     }
   }
+  return ExceptionStatus::kSuccess;
 }
 
 // Backwards lookup (slow).
@@ -8092,6 +8095,9 @@ HashTable<NameDictionary, NameDictionaryShape>::Shrink(Isolate* isolate,
                                                        Handle<NameDictionary>,
                                                        int additionalCapacity);
 
+template void HashTable<GlobalDictionary, GlobalDictionaryShape>::Rehash(
+    ReadOnlyRoots roots);
+
 Maybe<bool> JSFinalizationGroup::Cleanup(
     Isolate* isolate, Handle<JSFinalizationGroup> finalization_group,
     Handle<Object> cleanup) {

src/objects/ordered-hash-table.h

@@ -64,8 +64,8 @@ class OrderedHashTable : public FixedArray {
  public:
   // Returns an OrderedHashTable (possibly |table|) with enough space
   // to add at least one new element.
-  static Handle<Derived> EnsureGrowable(Isolate* isolate,
-                                        Handle<Derived> table);
+  static MaybeHandle<Derived> EnsureGrowable(Isolate* isolate,
+                                             Handle<Derived> table);
 
   // Returns an OrderedHashTable (possibly |table|) that's shrunken
   // if possible.
@@ -197,11 +197,11 @@ class OrderedHashTable : public FixedArray {
 
  protected:
   // Returns an OrderedHashTable with a capacity of at least |capacity|.
-  static Handle<Derived> Allocate(
+  static MaybeHandle<Derived> Allocate(
       Isolate* isolate, int capacity,
       AllocationType allocation = AllocationType::kYoung);
-  static Handle<Derived> Rehash(Isolate* isolate, Handle<Derived> table,
-                                int new_capacity);
+  static MaybeHandle<Derived> Rehash(Isolate* isolate, Handle<Derived> table,
+                                     int new_capacity);
 
   void SetNumberOfBuckets(int num) {
     set(NumberOfBucketsIndex(), Smi::FromInt(num));
@@ -235,16 +235,16 @@ class V8_EXPORT_PRIVATE OrderedHashSet
  public:
   DECL_CAST(OrderedHashSet)
 
-  static Handle<OrderedHashSet> Add(Isolate* isolate,
-                                    Handle<OrderedHashSet> table,
-                                    Handle<Object> value);
+  static MaybeHandle<OrderedHashSet> Add(Isolate* isolate,
+                                         Handle<OrderedHashSet> table,
+                                         Handle<Object> value);
   static Handle<FixedArray> ConvertToKeysArray(Isolate* isolate,
                                                Handle<OrderedHashSet> table,
                                                GetKeysConversion convert);
-  static Handle<OrderedHashSet> Rehash(Isolate* isolate,
-                                       Handle<OrderedHashSet> table,
-                                       int new_capacity);
-  static Handle<OrderedHashSet> Allocate(
+  static MaybeHandle<OrderedHashSet> Rehash(Isolate* isolate,
+                                            Handle<OrderedHashSet> table,
+                                            int new_capacity);
+  static MaybeHandle<OrderedHashSet> Allocate(
       Isolate* isolate, int capacity,
       AllocationType allocation = AllocationType::kYoung);
   static HeapObject GetEmpty(ReadOnlyRoots ro_roots);
@@ -262,16 +262,17 @@ class V8_EXPORT_PRIVATE OrderedHashMap
 
   // Returns a value if the OrderedHashMap contains the key, otherwise
   // returns undefined.
-  static Handle<OrderedHashMap> Add(Isolate* isolate,
-                                    Handle<OrderedHashMap> table,
-                                    Handle<Object> key, Handle<Object> value);
+  static MaybeHandle<OrderedHashMap> Add(Isolate* isolate,
+                                         Handle<OrderedHashMap> table,
+                                         Handle<Object> key,
+                                         Handle<Object> value);
 
-  static Handle<OrderedHashMap> Allocate(
+  static MaybeHandle<OrderedHashMap> Allocate(
       Isolate* isolate, int capacity,
       AllocationType allocation = AllocationType::kYoung);
-  static Handle<OrderedHashMap> Rehash(Isolate* isolate,
-                                       Handle<OrderedHashMap> table,
-                                       int new_capacity);
+  static MaybeHandle<OrderedHashMap> Rehash(Isolate* isolate,
+                                            Handle<OrderedHashMap> table,
+                                            int new_capacity);
   Object ValueAt(int entry);
 
   // This takes and returns raw Address values containing tagged Object
@@ -656,7 +657,7 @@ class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE) OrderedHashTableHandler {
  public:
   using Entry = int;
 
-  static Handle<HeapObject> Allocate(Isolate* isolate, int capacity);
+  static MaybeHandle<HeapObject> Allocate(Isolate* isolate, int capacity);
   static bool Delete(Handle<HeapObject> table, Handle<Object> key);
   static bool HasKey(Isolate* isolate, Handle<HeapObject> table,
                      Handle<Object> key);
@@ -672,9 +673,9 @@ extern template class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
 class V8_EXPORT_PRIVATE OrderedHashMapHandler
     : public OrderedHashTableHandler<SmallOrderedHashMap, OrderedHashMap> {
  public:
-  static Handle<HeapObject> Add(Isolate* isolate, Handle<HeapObject> table,
-                                Handle<Object> key, Handle<Object> value);
-  static Handle<OrderedHashMap> AdjustRepresentation(
+  static MaybeHandle<HeapObject> Add(Isolate* isolate, Handle<HeapObject> table,
+                                     Handle<Object> key, Handle<Object> value);
+  static MaybeHandle<OrderedHashMap> AdjustRepresentation(
       Isolate* isolate, Handle<SmallOrderedHashMap> table);
 };
 
@@ -684,9 +685,9 @@ extern template class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
 class V8_EXPORT_PRIVATE OrderedHashSetHandler
     : public OrderedHashTableHandler<SmallOrderedHashSet, OrderedHashSet> {
  public:
-  static Handle<HeapObject> Add(Isolate* isolate, Handle<HeapObject> table,
-                                Handle<Object> key);
-  static Handle<OrderedHashSet> AdjustRepresentation(
+  static MaybeHandle<HeapObject> Add(Isolate* isolate, Handle<HeapObject> table,
+                                     Handle<Object> key);
+  static MaybeHandle<OrderedHashSet> AdjustRepresentation(
       Isolate* isolate, Handle<SmallOrderedHashSet> table);
 };
 
@@ -695,7 +696,7 @@ class OrderedNameDictionary
  public:
   DECL_CAST(OrderedNameDictionary)
 
-  V8_EXPORT_PRIVATE static Handle<OrderedNameDictionary> Add(
+  V8_EXPORT_PRIVATE static MaybeHandle<OrderedNameDictionary> Add(
       Isolate* isolate, Handle<OrderedNameDictionary> table, Handle<Name> key,
       Handle<Object> value, PropertyDetails details);
 
@@ -705,11 +706,11 @@ class OrderedNameDictionary
   V8_EXPORT_PRIVATE static Handle<OrderedNameDictionary> DeleteEntry(
       Isolate* isolate, Handle<OrderedNameDictionary> table, int entry);
 
-  static Handle<OrderedNameDictionary> Allocate(
+  static MaybeHandle<OrderedNameDictionary> Allocate(
       Isolate* isolate, int capacity,
       AllocationType allocation = AllocationType::kYoung);
 
-  static Handle<OrderedNameDictionary> Rehash(
+  static MaybeHandle<OrderedNameDictionary> Rehash(
       Isolate* isolate, Handle<OrderedNameDictionary> table, int new_capacity);
 
   // Returns the value for entry.
@@ -745,9 +746,9 @@ class V8_EXPORT_PRIVATE OrderedNameDictionaryHandler
     : public OrderedHashTableHandler<SmallOrderedNameDictionary,
                                      OrderedNameDictionary> {
  public:
-  static Handle<HeapObject> Add(Isolate* isolate, Handle<HeapObject> table,
-                                Handle<Name> key, Handle<Object> value,
-                                PropertyDetails details);
+  static MaybeHandle<HeapObject> Add(Isolate* isolate, Handle<HeapObject> table,
+                                     Handle<Name> key, Handle<Object> value,
+                                     PropertyDetails details);
   static Handle<HeapObject> Shrink(Isolate* isolate, Handle<HeapObject> table);
 
   static Handle<HeapObject> DeleteEntry(Isolate* isolate,
@@ -779,7 +780,7 @@ class V8_EXPORT_PRIVATE OrderedNameDictionaryHandler
   static const int kNotFound = -1;
 
  protected:
-  static Handle<OrderedNameDictionary> AdjustRepresentation(
+  static MaybeHandle<OrderedNameDictionary> AdjustRepresentation(
       Isolate* isolate, Handle<SmallOrderedNameDictionary> table);
 };
 

src/runtime/runtime-collections.cc

@@ -25,7 +25,12 @@ RUNTIME_FUNCTION(Runtime_SetGrow) {
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(JSSet, holder, 0);
   Handle<OrderedHashSet> table(OrderedHashSet::cast(holder->table()), isolate);
-  table = OrderedHashSet::EnsureGrowable(isolate, table);
+  MaybeHandle<OrderedHashSet> table_candidate =
+      OrderedHashSet::EnsureGrowable(isolate, table);
+  if (!table_candidate.ToHandle(&table)) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewRangeError(MessageTemplate::kValueOutOfRange));
+  }
   holder->set_table(*table);
   return ReadOnlyRoots(isolate).undefined_value();
 }
@@ -56,7 +61,12 @@ RUNTIME_FUNCTION(Runtime_MapGrow) {
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(JSMap, holder, 0);
   Handle<OrderedHashMap> table(OrderedHashMap::cast(holder->table()), isolate);
-  table = OrderedHashMap::EnsureGrowable(isolate, table);
+  MaybeHandle<OrderedHashMap> table_candidate =
+      OrderedHashMap::EnsureGrowable(isolate, table);
+  if (!table_candidate.ToHandle(&table)) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewRangeError(MessageTemplate::kValueOutOfRange));
+  }
   holder->set_table(*table);
   return ReadOnlyRoots(isolate).undefined_value();
 }

test/mjsunit/mjsunit.status

@@ -182,6 +182,7 @@
   'regress/regress-605470': [PASS, SLOW],
   'regress/regress-655573': [PASS, SLOW],
   'regress/regress-1200351': [PASS, SLOW],
+  'regress/regress-crbug-918301': [PASS, SLOW, NO_VARIANTS, ['mode != release or dcheck_always_on', SKIP], ['(arch == arm or arch == arm64) and simulator_run', SKIP], ['tsan', SKIP]],
   'regress/wasm/regress-810973': [PASS, SLOW],
   'string-replace-gc': [PASS, SLOW],
   'wasm/asm-wasm-f32': [PASS, SLOW],

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

+// Copyright 2019 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.
+
+assertThrows(() => Object.getOwnPropertyDescriptors(Array(1e9).join('c')), RangeError);