[runtime] Add runtime checks for name collisions

Bug: chromium:1216437,chromium:1219630,chromium:1309225
Bug: chromium:1311641,chromium:1314616
Change-Id: I1575edbdd7fe91ed970ffe2f3437fd7c514e1ebd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3794525Reviewed-by: Samuel Groß <saelo@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Brendon Tiszka <tiszka@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82235}

src/objects/descriptor-array-inl.h

@@ -243,17 +243,24 @@ void DescriptorArray::Append(Descriptor* desc) {
   set_number_of_descriptors(descriptor_number + 1);
   Set(InternalIndex(descriptor_number), desc);
 
-  uint32_t hash = desc->GetKey()->hash();
+  uint32_t desc_hash = desc->GetKey()->hash();
+  // Hash value can't be zero, see String::ComputeAndSetHash()
+  uint32_t collision_hash = 0;
 
   int insertion;
 
   for (insertion = descriptor_number; insertion > 0; --insertion) {
     Name key = GetSortedKey(insertion - 1);
-    if (key.hash() <= hash) break;
+    collision_hash = key.hash();
+    if (collision_hash <= desc_hash) break;
     SetSortedKey(insertion, GetSortedKeyIndex(insertion - 1));
   }
 
   SetSortedKey(insertion, descriptor_number);
+
+  if (V8_LIKELY(collision_hash != desc_hash)) return;
+
+  CheckNameCollisionDuringInsertion(desc, desc_hash, insertion);
 }
 
 void DescriptorArray::SwapSortedKeys(int first, int second) {

src/objects/descriptor-array.h

@@ -120,6 +120,14 @@ class DescriptorArray
   // Sort the instance descriptors by the hash codes of their keys.
   V8_EXPORT_PRIVATE void Sort();
 
+  // Iterate through Name hash collisions in the descriptor array starting from
+  // insertion index checking for Name collisions. Note: If we ever add binary
+  // insertion for large DescriptorArrays it would need to be hardened in a
+  // similar way. This function only expects to be called on Sorted
+  // DescriptorArrays.
+  V8_EXPORT_PRIVATE void CheckNameCollisionDuringInsertion(
+      Descriptor* desc, uint32_t descriptor_hash, int insertion_index);
+
   // Search the instance descriptors for given name. {concurrent_search} signals
   // if we are doing the search on a background thread. If so, we will sacrifice
   // speed for thread-safety.

src/objects/objects.cc

@@ -4522,6 +4522,21 @@ void DescriptorArray::Sort() {
   DCHECK(IsSortedNoDuplicates());
 }
 
+void DescriptorArray::CheckNameCollisionDuringInsertion(Descriptor* desc,
+                                                        uint32_t desc_hash,
+                                                        int insertion_index) {
+  DCHECK_GE(insertion_index, 0);
+  DCHECK_LE(insertion_index, number_of_all_descriptors());
+
+  if (insertion_index <= 0) return;
+
+  for (int i = insertion_index; i > 0; --i) {
+    Name current_key = GetSortedKey(i - 1);
+    if (current_key.hash() != desc_hash) return;
+    CHECK(current_key != *desc->GetKey());
+  }
+}
+
 int16_t DescriptorArray::UpdateNumberOfMarkedDescriptors(
     unsigned mark_compact_epoch, int16_t new_marked) {
   static_assert(kMaxNumberOfDescriptors <=

src/objects/objects.h

@@ -547,7 +547,7 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
       Maybe<ShouldThrow> should_throw);
   V8_WARN_UNUSED_RESULT static Maybe<bool> SetDataProperty(
       LookupIterator* it, Handle<Object> value);
-  V8_WARN_UNUSED_RESULT static Maybe<bool> AddDataProperty(
+  V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe<bool> AddDataProperty(
       LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
       Maybe<ShouldThrow> should_throw, StoreOrigin store_origin,
       EnforceDefineSemantics semantics = EnforceDefineSemantics::kSet);

test/unittests/objects/object-unittest.cc

@@ -690,5 +690,71 @@ TEST_F(ObjectTest, ConstructorInstanceTypes) {
   }
 }
 
+TEST_F(ObjectTest, AddDataPropertyNameCollision) {
+  v8::HandleScope scope(isolate());
+  Factory* factory = i_isolate()->factory();
+
+  Handle<JSObject> object =
+      factory->NewJSObject(i_isolate()->object_function());
+
+  Handle<String> key = factory->NewStringFromStaticChars("key_string");
+  Handle<Object> value1(Smi::FromInt(0), i_isolate());
+  Handle<Object> value2 = factory->NewStringFromAsciiChecked("corrupt");
+
+  LookupIterator outer_it(i_isolate(), object, key, object,
+                          LookupIterator::OWN_SKIP_INTERCEPTOR);
+  {
+    LookupIterator inner_it(i_isolate(), object, key, object,
+                            LookupIterator::OWN_SKIP_INTERCEPTOR);
+
+    CHECK(Object::AddDataProperty(&inner_it, value1, NONE,
+                                  Just(ShouldThrow::kThrowOnError),
+                                  StoreOrigin::kNamed)
+              .IsJust());
+  }
+  EXPECT_DEATH_IF_SUPPORTED(
+      Object::AddDataProperty(&outer_it, value2, NONE,
+                              Just(ShouldThrow::kThrowOnError),
+                              StoreOrigin::kNamed)
+          .IsJust(),
+      "");
+}
+
+TEST_F(ObjectTest, AddDataPropertyNameCollisionDeprecatedMap) {
+  v8::HandleScope scope(isolate());
+  Factory* factory = i_isolate()->factory();
+
+  // Create two identical maps
+  RunJS(
+      "a = {'regular_prop':5};"
+      "b = {'regular_prop':5};");
+
+  Handle<JSObject> a = Handle<JSObject>::cast(v8::Utils::OpenHandle(
+      *context()->Global()->Get(context(), NewString("a")).ToLocalChecked()));
+  Handle<JSObject> b = Handle<JSObject>::cast(v8::Utils::OpenHandle(
+      *context()->Global()->Get(context(), NewString("b")).ToLocalChecked()));
+
+  CHECK(a->map() == b->map());
+
+  Handle<String> key = factory->NewStringFromStaticChars("corrupted_prop");
+  Handle<Object> value = factory->NewStringFromAsciiChecked("corrupt");
+  LookupIterator it(i_isolate(), a, key, a,
+                    LookupIterator::OWN_SKIP_INTERCEPTOR);
+
+  // Transition `a`'s map to deprecated
+  RunJS(
+      "a.corrupted_prop = 1;"
+      "b.regular_prop = 5.5;");
+
+  CHECK(a->map().is_deprecated());
+
+  EXPECT_DEATH_IF_SUPPORTED(
+      Object::AddDataProperty(&it, value, NONE,
+                              Just(ShouldThrow::kThrowOnError),
+                              StoreOrigin::kNamed)
+          .IsJust(),
+      "");
+}
+
 }  // namespace internal
 }  // namespace v8