[sandbox] Fix ExternalString --> ThinString transition

pthier · V8 LUCI CQ · commit 524e681b5d9d · 2022-10-06T08:52:29.000Z
When an ExternalString is transitioned to a ThinString, we need to notify the GC about the layout change. In addition we need to use slot snapshotting for external strings in concurrent marking to avoid interpreting stale slots as external pointers. Bug: chromium:1370303 Change-Id: Ibcf6c1eafb31df392d97a4761e006b9d3507bd5f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3936151 Reviewed-by: Dominik Inführ <dinfuehr@chromium.org> Commit-Queue: Patrick Thier <pthier@chromium.org> Cr-Commit-Position: refs/heads/main@{#83552}
diff --git a/src/heap/concurrent-marking.cc b/src/heap/concurrent-marking.cc
@@ -68,21 +68,48 @@ class ConcurrentMarkingState final
 // Helper class for storing in-object slot addresses and values.
 class SlotSnapshot {
  public:
-  SlotSnapshot() : number_of_slots_(0) {}
+  SlotSnapshot()
+      : number_of_object_slots_(0), number_of_external_pointer_slots_(0) {}
   SlotSnapshot(const SlotSnapshot&) = delete;
   SlotSnapshot& operator=(const SlotSnapshot&) = delete;
-  int number_of_slots() const { return number_of_slots_; }
-  ObjectSlot slot(int i) const { return snapshot_[i].first; }
-  Object value(int i) const { return snapshot_[i].second; }
-  void clear() { number_of_slots_ = 0; }
+  int number_of_object_slots() const { return number_of_object_slots_; }
+  int number_of_external_pointer_slots() const {
+    return number_of_external_pointer_slots_;
+  }
+  ObjectSlot object_slot(int i) const { return object_snapshot_[i].first; }
+  Object object_value(int i) const { return object_snapshot_[i].second; }
+  ExternalPointerSlot external_pointer_slot(int i) const {
+    return external_pointer_snapshot_[i].first;
+  }
+  ExternalPointerTag external_pointer_tag(int i) const {
+    return external_pointer_snapshot_[i].second;
+  }
+  void clear() {
+    number_of_object_slots_ = 0;
+    number_of_external_pointer_slots_ = 0;
+  }
   void add(ObjectSlot slot, Object value) {
-    snapshot_[number_of_slots_++] = {slot, value};
+    DCHECK_LT(number_of_object_slots_, kMaxObjectSlots);
+    object_snapshot_[number_of_object_slots_++] = {slot, value};
+  }
+  void add(ExternalPointerSlot slot, ExternalPointerTag tag) {
+    DCHECK_LT(number_of_external_pointer_slots_, kMaxExternalPointerSlots);
+    external_pointer_snapshot_[number_of_external_pointer_slots_++] = {slot,
+                                                                       tag};
   }
 
  private:
-  static const int kMaxSnapshotSize = JSObject::kMaxInstanceSize / kTaggedSize;
-  int number_of_slots_;
-  std::pair<ObjectSlot, Object> snapshot_[kMaxSnapshotSize];
+  // Maximum number of pointer slots of objects we use snapshotting for.
+  // ConsStrings can have 3 (Map + Left + Right) pointers.
+  static constexpr int kMaxObjectSlots = 3;
+  // Maximum number of external pointer slots of objects we use snapshotting
+  // for. ExternalStrings can have 2 (resource + cached data) external pointers.
+  static constexpr int kMaxExternalPointerSlots = 2;
+  int number_of_object_slots_;
+  int number_of_external_pointer_slots_;
+  std::pair<ObjectSlot, Object> object_snapshot_[kMaxObjectSlots];
+  std::pair<ExternalPointerSlot, ExternalPointerTag>
+      external_pointer_snapshot_[kMaxExternalPointerSlots];
 };
 
 class ConcurrentMarkingVisitorUtility {
@@ -114,9 +141,9 @@ class ConcurrentMarkingVisitorUtility {
   template <typename Visitor>
   static void VisitPointersInSnapshot(Visitor* visitor, HeapObject host,
                                       const SlotSnapshot& snapshot) {
-    for (int i = 0; i < snapshot.number_of_slots(); i++) {
-      ObjectSlot slot = snapshot.slot(i);
-      Object object = snapshot.value(i);
+    for (int i = 0; i < snapshot.number_of_object_slots(); i++) {
+      ObjectSlot slot = snapshot.object_slot(i);
+      Object object = snapshot.object_value(i);
       DCHECK(!HasWeakHeapObjectTag(object));
       if (!object.IsHeapObject()) continue;
       HeapObject heap_object = HeapObject::cast(object);
@@ -127,6 +154,16 @@ class ConcurrentMarkingVisitorUtility {
     }
   }
 
+  template <typename Visitor>
+  static void VisitExternalPointersInSnapshot(Visitor* visitor, HeapObject host,
+                                              const SlotSnapshot& snapshot) {
+    for (int i = 0; i < snapshot.number_of_external_pointer_slots(); i++) {
+      ExternalPointerSlot slot = snapshot.external_pointer_slot(i);
+      ExternalPointerTag tag = snapshot.external_pointer_tag(i);
+      visitor->VisitExternalPointer(host, slot, tag);
+    }
+  }
+
   template <typename Visitor, typename T>
   static int VisitFullyWithSnapshot(Visitor* visitor, Map map, T object) {
     using TBodyDescriptor = typename T::BodyDescriptor;
@@ -137,6 +174,8 @@ class ConcurrentMarkingVisitorUtility {
     if (!visitor->ShouldVisit(object)) return 0;
     ConcurrentMarkingVisitorUtility::VisitPointersInSnapshot(visitor, object,
                                                              snapshot);
+    ConcurrentMarkingVisitorUtility::VisitExternalPointersInSnapshot(
+        visitor, object, snapshot);
     return size;
   }
 
@@ -183,6 +222,11 @@ class ConcurrentMarkingVisitorUtility {
       UNREACHABLE();
     }
 
+    void VisitExternalPointer(HeapObject host, ExternalPointerSlot slot,
+                              ExternalPointerTag tag) override {
+      slot_snapshot_->add(slot, tag);
+    }
+
     void VisitCodeTarget(Code host, RelocInfo* rinfo) final {
       // This should never happen, because snapshotting is performed only on
       // some String subclasses.
@@ -453,6 +497,16 @@ class ConcurrentMarkingVisitor final
     return SeqTwoByteString::SizeFor(object.length(kAcquireLoad));
   }
 
+  int VisitExternalOneByteString(Map map, ExternalOneByteString object) {
+    return ConcurrentMarkingVisitorUtility::VisitFullyWithSnapshot(this, map,
+                                                                   object);
+  }
+
+  int VisitExternalTwoByteString(Map map, ExternalTwoByteString object) {
+    return ConcurrentMarkingVisitorUtility::VisitFullyWithSnapshot(this, map,
+                                                                   object);
+  }
+
   // Implements ephemeron semantics: Marks value if key is already reachable.
   // Returns true if value was actually marked.
   bool ProcessEphemeron(HeapObject key, HeapObject value) {
diff --git a/src/objects/string.cc b/src/objects/string.cc
@@ -223,6 +223,14 @@ void String::MakeThin(IsolateT* isolate, String internalized) {
   Map target_map = ComputeThinStringMap(isolate, initial_shape,
                                         internalized.IsOneByteRepresentation());
   if (initial_shape.IsExternal()) {
+    // Notify GC about the layout change before the transition to avoid
+    // concurrent marking from observing any in-between state (e.g.
+    // ExternalString map where the resource external pointer is overwritten
+    // with a tagged pointer).
+    // ExternalString -> ThinString transitions can only happen on the
+    // main-thread.
+    isolate->AsIsolate()->heap()->NotifyObjectLayoutChange(
+        *this, no_gc, InvalidateRecordedSlots::kYes, ThinString::kSize);
     MigrateExternalString(isolate->AsIsolate(), *this, internalized);
   }
 
@@ -231,7 +239,11 @@ void String::MakeThin(IsolateT* isolate, String internalized) {
   // ThinString.
   ThinString thin = ThinString::unchecked_cast(*this);
   thin.set_actual(internalized);
-  set_map_safe_transition(target_map, kReleaseStore);
+  if (initial_shape.IsExternal()) {
+    set_map(target_map, kReleaseStore);
+  } else {
+    set_map_safe_transition(target_map, kReleaseStore);
+  }
 
   DCHECK_GE(old_size, ThinString::kSize);
   int size_delta = old_size - ThinString::kSize;
