[Backport] Security bug 1201938

Manual cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/v8/v8/+/2880214: Merged: [const-tracking] Generalize constness when delete properties Revision: d570bbe0c74ec4ae40d1abc34bea617ff2d63f26 BUG=chromium:1201938 NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true [email protected] Change-Id: I2745bd574d9f971b3f1e41d5084ec9e9fbbeef07 Reviewed-by: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/branch-heads/9.0@{#55} Cr-Branched-From: bd0108b4c88e0d6f2350cb79b5f363fbd02f3eb7-refs/heads/9.0.257@{#1} Cr-Branched-From: 349bcc6a075411f1a7ce2d866c3dfeefc2efa39d-refs/heads/master@{#73001} Reviewed-by: Allan Sandfeld Jensen <[email protected]>
qt · May 28, 2021 · eaffb82 · eaffb82
1 parent 951cdb3
commit eaffb82
Show file tree

Hide file tree

Showing 11 changed files with 171 additions and 71 deletions.
diff --git a/chromium/v8/src/base/platform/mutex.h b/chromium/v8/src/base/platform/mutex.h
@@ -11,6 +11,7 @@
 #include "src/base/win32-headers.h"
 #endif
 #include "src/base/logging.h"
+#include "src/base/optional.h"
 
 #if V8_OS_POSIX
 #include <pthread.h>  // NOLINT
@@ -340,6 +341,20 @@ class SharedMutexGuard final {
   DISALLOW_COPY_AND_ASSIGN(SharedMutexGuard);
 };
 
+template <MutexSharedType kIsShared,
+          NullBehavior Behavior = NullBehavior::kRequireNotNull>
+class SharedMutexGuardIf final {
+ public:
+  SharedMutexGuardIf(SharedMutex* mutex, bool enable_mutex) {
+    if (enable_mutex) mutex_.emplace(mutex);
+  }
+
+ private:
+  base::Optional<SharedMutexGuard<kIsShared, Behavior>> mutex_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedMutexGuardIf);
+};
+
 }  // namespace base
 }  // namespace v8
 

diff --git a/chromium/v8/src/builtins/builtins-internal-gen.cc b/chromium/v8/src/builtins/builtins-internal-gen.cc
@@ -508,8 +508,8 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) {
     Label dictionary(this), dont_delete(this);
     GotoIf(IsDictionaryMap(receiver_map), &dictionary);
 
-    // Fast properties need to clear recorded slots, which can only be done
-    // in C++.
+    // Fast properties need to clear recorded slots and mark the deleted
+    // property as mutable, which can only be done in C++.
     Goto(&slow);
 
     BIND(&dictionary);

diff --git a/chromium/v8/src/objects/internal-index.h b/chromium/v8/src/objects/internal-index.h
@@ -45,11 +45,11 @@ class InternalIndex {
     return static_cast<int>(entry_);
   }
 
-  bool operator==(const InternalIndex& other) { return entry_ == other.entry_; }
+  bool operator==(const InternalIndex& other) const { return entry_ == other.entry_; }
 
   // Iteration support.
   InternalIndex operator*() { return *this; }
-  bool operator!=(const InternalIndex& other) { return entry_ != other.entry_; }
+  bool operator!=(const InternalIndex& other) const { return entry_ != other.entry_; }
   InternalIndex& operator++() {
     entry_++;
     return *this;

diff --git a/chromium/v8/src/objects/map-inl.h b/chromium/v8/src/objects/map-inl.h
@@ -185,6 +185,10 @@ bool Map::TooManyFastProperties(StoreOrigin store_origin) const {
   }
 }
 
+Name Map::GetLastDescriptorName(Isolate* isolate) const {
+  return instance_descriptors(isolate).GetKey(LastAdded());
+}
+
 PropertyDetails Map::GetLastDescriptorDetails(Isolate* isolate) const {
   return instance_descriptors(isolate).GetDetails(LastAdded());
 }

diff --git a/chromium/v8/src/objects/map-updater.cc b/chromium/v8/src/objects/map-updater.cc
@@ -121,50 +121,6 @@ Handle<Map> MapUpdater::ReconfigureToDataField(InternalIndex descriptor,
   PropertyDetails old_details =
       old_descriptors_->GetDetails(modified_descriptor_);
 
-  // If the {descriptor} was "const" data field so far, we need to update the
-  // {old_map_} here, otherwise we could get the constants wrong, i.e.
-  //
-  //   o.x = 1;
-  //   change o.x's attributes to something else
-  //   delete o.x;
-  //   o.x = 2;
-  //
-  // could trick V8 into thinking that `o.x` is still 1 even after the second
-  // assignment.
-  // This situation is similar to what might happen with property deletion.
-  if (old_details.constness() == PropertyConstness::kConst &&
-      old_details.location() == kField &&
-      old_details.attributes() != new_attributes_) {
-    // Ensure we'll be updating constness of the up-to-date version of old_map_.
-    Handle<Map> old_map = Map::Update(isolate_, old_map_);
-    PropertyDetails details =
-        old_map->instance_descriptors().GetDetails(descriptor);
-    Handle<FieldType> field_type(
-        old_map->instance_descriptors().GetFieldType(descriptor),
-        isolate_);
-    Map::GeneralizeField(isolate_, old_map, descriptor,
-                         PropertyConstness::kMutable, details.representation(),
-                         field_type);
-    DCHECK_EQ(PropertyConstness::kMutable,
-              old_map->instance_descriptors()
-                  .GetDetails(descriptor)
-                  .constness());
-    // The old_map_'s property must become mutable.
-    // Note, that the {old_map_} and {old_descriptors_} are not expected to be
-    // updated by the generalization if the map is already deprecated.
-    DCHECK_IMPLIES(
-        !old_map_->is_deprecated(),
-        PropertyConstness::kMutable ==
-            old_descriptors_->GetDetails(modified_descriptor_).constness());
-    // Although the property in the old map is marked as mutable we still
-    // treat it as constant when merging with the new path in transition tree.
-    // This is fine because up until this reconfiguration the field was
-    // known to be constant, so it's fair to proceed treating it as such
-    // during this reconfiguration session. The issue is that after the
-    // reconfiguration the original field might become mutable (see the delete
-    // example above).
-  }
-
   // If property kind is not reconfigured merge the result with
   // representation/field type from the old descriptor.
   if (old_details.kind() == new_kind_) {

diff --git a/chromium/v8/src/objects/map.h b/chromium/v8/src/objects/map.h
@@ -639,6 +639,7 @@ class Map : public HeapObject {
   // chain state.
   inline bool IsPrototypeValidityCellValid() const;
 
+  inline Name GetLastDescriptorName(Isolate* isolate) const;
   inline PropertyDetails GetLastDescriptorDetails(Isolate* isolate) const;
 
   inline InternalIndex LastAdded() const;

diff --git a/chromium/v8/src/objects/property-details.h b/chromium/v8/src/objects/property-details.h
@@ -32,6 +32,12 @@ enum PropertyAttributes {
   // a non-existent property.
 };
 
+// Number of distinct bits in PropertyAttributes.
+static const int kPropertyAttributesBitsCount = 3;
+
+static const int kPropertyAttributesCombinationsCount =
+    1 << kPropertyAttributesBitsCount;
+
 enum PropertyFilter {
   ALL_PROPERTIES = 0,
   ONLY_WRITABLE = 1,
@@ -63,6 +69,11 @@ STATIC_ASSERT(SKIP_STRINGS ==
 STATIC_ASSERT(SKIP_SYMBOLS ==
               static_cast<PropertyFilter>(v8::PropertyFilter::SKIP_SYMBOLS));
 
+// Assert that kPropertyAttributesBitsCount value matches the definition of
+// ALL_ATTRIBUTES_MASK.
+STATIC_ASSERT((ALL_ATTRIBUTES_MASK == (READ_ONLY | DONT_ENUM | DONT_DELETE)) ==
+              (kPropertyAttributesBitsCount == 3));
+
 class Smi;
 class TypeInfo;
 

diff --git a/chromium/v8/src/objects/transitions.cc b/chromium/v8/src/objects/transitions.cc
@@ -272,6 +272,34 @@ MaybeHandle<Map> TransitionsAccessor::FindTransitionToDataProperty(
   return Handle<Map>(target, isolate_);
 }
 
+void TransitionsAccessor::ForEachTransitionTo(
+    Name name, const ForEachTransitionCallback& callback,
+    DisallowHeapAllocation* no_gc) {
+  DCHECK(name.IsUniqueName());
+  switch (encoding()) {
+    case kPrototypeInfo:
+    case kUninitialized:
+    case kMigrationTarget:
+      return;
+    case kWeakRef: {
+      Map target = Map::cast(raw_transitions_->GetHeapObjectAssumeWeak());
+      InternalIndex descriptor = target.LastAdded();
+      DescriptorArray descriptors = target.instance_descriptors();
+      Name key = descriptors.GetKey(descriptor);
+      if (key == name) {
+        callback(target);
+      }
+      return;
+    }
+    case kFullTransitionArray: {
+      v8::base::SharedMutexGuardIf<base::kShared> scope(
+          isolate_->transition_array_access(), concurrent_access_);
+      return transitions().ForEachTransitionTo(name, callback);
+    }
+  }
+  UNREACHABLE();
+}
+
 bool TransitionsAccessor::CanHaveMoreTransitions() {
   if (map_.is_dictionary_map()) return false;
   if (encoding() == kFullTransitionArray) {
@@ -613,6 +641,21 @@ Map TransitionArray::SearchAndGetTarget(PropertyKind kind, Name name,
   return SearchDetailsAndGetTarget(transition, kind, attributes);
 }
 
+void TransitionArray::ForEachTransitionTo(
+    Name name, const ForEachTransitionCallback& callback) {
+  int transition = SearchName(name, nullptr);
+  if (transition == kNotFound) return;
+
+  int nof_transitions = number_of_transitions();
+  DCHECK(transition < nof_transitions);
+  Name key = GetKey(transition);
+  for (; transition < nof_transitions && GetKey(transition) == key;
+       transition++) {
+    Map target = GetTarget(transition);
+    callback(target);
+  }
+}
+
 void TransitionArray::Sort() {
   DisallowHeapAllocation no_gc;
   // In-place insertion sort.

diff --git a/chromium/v8/src/objects/transitions.h b/chromium/v8/src/objects/transitions.h
@@ -19,6 +19,9 @@
 namespace v8 {
 namespace internal {
 
+// Find all transitions with given name and calls the callback.
+using ForEachTransitionCallback = std::function<void(Map)>;
+
 // TransitionsAccessor is a helper class to encapsulate access to the various
 // ways a Map can store transitions to other maps in its respective field at
 // Map::kTransitionsOrPrototypeInfo.
@@ -68,6 +71,14 @@ class V8_EXPORT_PRIVATE TransitionsAccessor {
     return FindTransitionToDataProperty(name, kFieldOnly);
   }
 
+  // Find all transitions with given name and calls the callback.
+  // Neither GCs nor operations requiring Isolate::full_transition_array_access
+  // lock are allowed inside the callback.
+  // If any of the GC- or lock-requiring processing is necessary, it has to be
+  // done outside of the callback.
+  void ForEachTransitionTo(Name name, const ForEachTransitionCallback& callback,
+                           DisallowHeapAllocation* no_gc);
+
   inline Handle<String> ExpectedTransitionKey();
   inline Handle<Map> ExpectedTransitionTarget();
 
@@ -320,6 +331,10 @@ class TransitionArray : public WeakFixedArray {
   Map SearchDetailsAndGetTarget(int transition, PropertyKind kind,
                                 PropertyAttributes attributes);
 
+  // Find all transitions with given name and calls the callback.
+  void ForEachTransitionTo(Name name,
+                           const ForEachTransitionCallback& callback);
+
   inline int number_of_transitions() const;
 
   static bool CompactPrototypeTransitionArray(Isolate* isolate,

diff --git a/chromium/v8/src/runtime/runtime-object.cc b/chromium/v8/src/runtime/runtime-object.cc
@@ -17,6 +17,7 @@
 #include "src/objects/js-array-inl.h"
 #include "src/objects/property-descriptor-object.h"
 #include "src/objects/property-descriptor.h"
+#include "src/objects/property-details.h"
 #include "src/runtime/runtime-utils.h"
 #include "src/runtime/runtime.h"
 
@@ -93,6 +94,54 @@ MaybeHandle<Object> Runtime::HasProperty(Isolate* isolate,
 
 namespace {
 
+void GeneralizeAllTransitionsToFieldAsMutable(Isolate* isolate, Handle<Map> map,
+                                              Handle<Name> name) {
+  InternalIndex descriptor(map->NumberOfOwnDescriptors());
+
+  Handle<Map> target_maps[kPropertyAttributesCombinationsCount];
+  int target_maps_count = 0;
+
+  // Collect all outgoing field transitions.
+  {
+    DisallowHeapAllocation no_gc;
+    TransitionsAccessor transitions(isolate, *map, &no_gc);
+    transitions.ForEachTransitionTo(
+        *name,
+        [&](Map target) {
+          DCHECK_EQ(descriptor, target.LastAdded());
+          DCHECK_EQ(*name, target.GetLastDescriptorName(isolate));
+          PropertyDetails details = target.GetLastDescriptorDetails(isolate);
+          // Currently, we track constness only for fields.
+          if (details.kind() == kData &&
+              details.constness() == PropertyConstness::kConst) {
+            target_maps[target_maps_count++] = handle(target, isolate);
+          }
+          DCHECK_IMPLIES(details.kind() == kAccessor,
+                         details.constness() == PropertyConstness::kConst);
+        },
+        &no_gc);
+    CHECK_LE(target_maps_count, kPropertyAttributesCombinationsCount);
+  }
+
+  for (int i = 0; i < target_maps_count; i++) {
+    Handle<Map> target = target_maps[i];
+    PropertyDetails details =
+        target->instance_descriptors(isolate)
+            .GetDetails(descriptor);
+    Handle<FieldType> field_type(
+        target->instance_descriptors(isolate)
+            .GetFieldType(descriptor),
+        isolate);
+    Map::GeneralizeField(isolate, target, descriptor,
+                         PropertyConstness::kMutable, details.representation(),
+                         field_type);
+    DCHECK_EQ(PropertyConstness::kMutable,
+              target->instance_descriptors(isolate)
+                  .GetDetails(descriptor)
+                  .constness());
+  }
+}
+
 bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
                               Handle<Object> raw_key) {
   // This implements a special case for fast property deletion: when the
@@ -102,6 +151,8 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
   // (1) The receiver must be a regular object and the key a unique name.
   Handle<Map> receiver_map(receiver->map(), isolate);
   if (receiver_map->IsSpecialReceiverMap()) return false;
+  DCHECK(receiver_map->IsJSObjectMap());
+
   if (!raw_key->IsUniqueName()) return false;
   Handle<Name> key = Handle<Name>::cast(raw_key);
   // (2) The property to be deleted must be the last property.
@@ -124,26 +175,6 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
 
   // Preconditions successful. No more bailouts after this point.
 
-  // If the {descriptor} was "const" so far, we need to update the
-  // {receiver_map} here, otherwise we could get the constants wrong, i.e.
-  //
-  //   o.x = 1;
-  //   delete o.x;
-  //   o.x = 2;
-  //
-  // could trick V8 into thinking that `o.x` is still 1 even after the second
-  // assignment.
-  if (details.constness() == PropertyConstness::kConst &&
-      details.location() == kField) {
-    Handle<FieldType> field_type(descriptors->GetFieldType(descriptor),
-                                 isolate);
-    Map::GeneralizeField(isolate, receiver_map, descriptor,
-                         PropertyConstness::kMutable, details.representation(),
-                         field_type);
-    DCHECK_EQ(PropertyConstness::kMutable,
-              descriptors->GetDetails(descriptor).constness());
-  }
-
   // Zap the property to avoid keeping objects alive. Zapping is not necessary
   // for properties stored in the descriptor array.
   if (details.location() == kField) {
@@ -190,6 +221,30 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
   receiver->HeapObjectVerify(isolate);
   receiver->property_array().PropertyArrayVerify(isolate);
 #endif
+
+  // If the {descriptor} was "const" so far, we need to update the
+  // {receiver_map} here, otherwise we could get the constants wrong, i.e.
+  //
+  //   o.x = 1;
+  //   [change o.x's attributes or reconfigure property kind]
+  //   delete o.x;
+  //   o.x = 2;
+  //
+  // could trick V8 into thinking that `o.x` is still 1 even after the second
+  // assignment.
+
+  // Step 1: Migrate object to an up-to-date shape.
+  if (parent_map->is_deprecated()) {
+    JSObject::MigrateInstance(isolate, Handle<JSObject>::cast(receiver));
+    parent_map = handle(receiver->map(), isolate);
+  }
+
+  // Step 2: Mark outgoing transitions from the up-to-date version of the
+  // parent_map to same property name of any kind or attributes as mutable.
+  // Also migrate object to the up-to-date map to make the object shapes
+  // converge sooner.
+  GeneralizeAllTransitionsToFieldAsMutable(isolate, parent_map, key);
+
   return true;
 }
 

diff --git a/chromium/v8/src/runtime/runtime.h b/chromium/v8/src/runtime/runtime.h
@@ -749,9 +749,9 @@ class Runtime : public AllStatic {
   // Get the runtime intrinsic function table.
   static const Function* RuntimeFunctionTable(Isolate* isolate);
 
-  V8_WARN_UNUSED_RESULT static Maybe<bool> DeleteObjectProperty(
-      Isolate* isolate, Handle<JSReceiver> receiver, Handle<Object> key,
-      LanguageMode language_mode);
+  V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe<bool>
+  DeleteObjectProperty(Isolate* isolate, Handle<JSReceiver> receiver,
+                       Handle<Object> key, LanguageMode language_mode);
 
   V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
   SetObjectProperty(Isolate* isolate, Handle<Object> object, Handle<Object> key,