From 493ea0b937d9a6c479d5358a5dced227bdf0fd5c Mon Sep 17 00:00:00 2001
From: Michael Lippautz <[email protected]>
Date: Mon, 15 Jun 2026 20:03:25 +0000
Subject: [PATCH] Reland "sandbox: Fix duplicate creation of evacuation entries"

This is a reland of commit 0f9a757a7da9e4701d6b6402d888a8f0d195ac33

Original change's description:
> sandbox: Fix duplicate creation of evacuation entries
>
> When marking pointers in CppHeapPointerTable and ExternalPointerTable,
> a race with a concurrent mutator write could cause the mark bit to be
> cleared and then set again, generating duplicate evacuation entries
> for the same handle location. Sweeping then crashed when processing
> the second entry because the handle had already been updated to the
> new (low) index, failing the old_index >= start_of_evacuation_area
> assertion.
>
> This also removes black allocation (automatically always setting the
> mark bit) for the EPT.
>
> TAG=agy
>
> Bug: 508200984
> Change-Id: I21060784886b4c5a18ac75862b99cca9b1d62981
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7915143
> Reviewed-by: Samuel Groß <[email protected]>
> Commit-Queue: Michael Lippautz <[email protected]>
> Reviewed-by: Arash Kazemi <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#107961}

Bug: 508200984, 524290062
Change-Id: I8668c4eeb9bc8296fe75687c6d08adf47b5ca457
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7943563
Auto-Submit: Michael Lippautz <[email protected]>
Commit-Queue: Arash Kazemi <[email protected]>
Reviewed-by: Samuel Groß <[email protected]>
Reviewed-by: Arash Kazemi <[email protected]>
Cr-Commit-Position: refs/heads/main@{#108012}
---

diff --git a/src/heap/heap-write-barrier.cc b/src/heap/heap-write-barrier.cc
index 9790f7a..03cfc9c 100644
--- a/src/heap/heap-write-barrier.cc
+++ b/src/heap/heap-write-barrier.cc
@@ -160,6 +160,13 @@
 
   ExternalPointerHandle handle = slot.Relaxed_LoadHandle();
   table.Mark(space, handle, slot.address());
+
+  if (marking_barrier->is_minor() && HeapLayout::InYoungGeneration(host)) {
+    MutablePage* host_page =
+        MutablePage::FromHeapObject(marking_barrier->heap()->isolate(), host);
+    RememberedSet<SURVIVOR_TO_EXTERNAL_POINTER>::Insert<AccessMode::ATOMIC>(
+        host_page, host_page->Offset(slot.address()));
+  }
 #endif  // V8_COMPRESS_POINTERS
 }
 
diff --git a/src/sandbox/cppheap-pointer-table-inl.h b/src/sandbox/cppheap-pointer-table-inl.h
index ffc153e..b477196 100644
--- a/src/sandbox/cppheap-pointer-table-inl.h
+++ b/src/sandbox/cppheap-pointer-table-inl.h
@@ -44,10 +44,18 @@
   DCHECK_EQ(0, value >> (kBitsPerSystemPointer - kCppHeapPointerPayloadShift));
   DCHECK_NE(tag, CppHeapPointerTag::kFreeEntryTag);
   DCHECK_NE(tag, CppHeapPointerTag::kEvacuationEntryTag);
-  DCHECK(payload_.load(std::memory_order_relaxed).ContainsPointer());
-
-  Payload new_payload(value, tag);
-  payload_.store(new_payload, std::memory_order_relaxed);
+  auto old_payload = payload_.load(std::memory_order_relaxed);
+  while (true) {
+    DCHECK(old_payload.ContainsPointer());
+    Payload new_payload(value, tag);
+    if (old_payload.HasMarkBitSet()) {
+      new_payload.SetMarkBit();
+    }
+    if (payload_.compare_exchange_weak(old_payload, new_payload,
+                                       std::memory_order_relaxed)) {
+      break;
+    }
+  }
 }
 
 bool CppHeapPointerTableEntry::HasPointer(
@@ -73,18 +81,20 @@
   return payload.ExtractFreelistLink();
 }
 
-void CppHeapPointerTableEntry::Mark() {
+bool CppHeapPointerTableEntry::Mark() {
   auto old_payload = payload_.load(std::memory_order_relaxed);
-  DCHECK(old_payload.ContainsPointer());
-
-  auto new_payload = old_payload;
-  new_payload.SetMarkBit();
-
-  // We don't need to perform the CAS in a loop: if the new value is not equal
-  // to the old value, then the mutator must've just written a new value into
-  // the entry. The mutator will also set the markbit through the write barrier.
-  payload_.compare_exchange_strong(old_payload, new_payload,
-                                   std::memory_order_relaxed);
+  while (true) {
+    DCHECK(old_payload.ContainsPointer());
+    if (old_payload.HasMarkBitSet()) {
+      return false;
+    }
+    auto new_payload = old_payload;
+    new_payload.SetMarkBit();
+    if (payload_.compare_exchange_weak(old_payload, new_payload,
+                                       std::memory_order_relaxed)) {
+      return true;
+    }
+  }
 }
 
 void CppHeapPointerTableEntry::MakeEvacuationEntry(Address handle_location) {
@@ -160,13 +170,16 @@
   uint32_t index = HandleToIndex(handle);
   DCHECK(space->Contains(index));
 
-  // If the table is being compacted and the entry is inside the evacuation
-  // area, then allocate and set up an evacuation entry for it.
-  MaybeCreateEvacuationEntry(space, index, handle_location);
+  // Bail out in case the entry was already marked.
+  if (!at(index).Mark()) {
+    return;
+  }
 
-  // Even if the entry is marked for evacuation, it still needs to be marked as
-  // alive as it may be visited during sweeping before being evacuation.
-  at(index).Mark();
+  // If the table is being compacted and the entry is inside the evacuation
+  // area, then allocate and set up an evacuation entry for it. Only one such
+  // entry must exist for any given `handle_location` which is ensured by
+  // properly bailing out on marked entries above.
+  MaybeCreateEvacuationEntry(space, index, handle_location);
 }
 
 // static
diff --git a/src/sandbox/cppheap-pointer-table.h b/src/sandbox/cppheap-pointer-table.h
index 0ef2e7a..34ee242 100644
--- a/src/sandbox/cppheap-pointer-table.h
+++ b/src/sandbox/cppheap-pointer-table.h
@@ -81,8 +81,9 @@
   // Invalidates the source entry.
   inline void Evacuate(CppHeapPointerTableEntry& dest);
 
-  // Mark this entry as alive during table garbage collection.
-  inline void Mark();
+  // Mark this entry as alive during table garbage collection. Returns true if
+  // the entry transitioned from un-marked to marked, and false otherwise.
+  inline bool Mark();
 
   static constexpr bool IsWriteProtected = false;
 
diff --git a/src/sandbox/external-pointer-table-inl.h b/src/sandbox/external-pointer-table-inl.h
index d0fce98..6f12a37 100644
--- a/src/sandbox/external-pointer-table-inl.h
+++ b/src/sandbox/external-pointer-table-inl.h
@@ -45,11 +45,17 @@
   DCHECK_EQ(0, value & kExternalPointerTagAndMarkbitMask);
   DCHECK(payload_.load(std::memory_order_relaxed).ContainsPointer());
 
-  Payload new_payload(value, tag);
-  // Writing an entry currently also marks it as alive. In the future, we might
-  // want to drop this and instead use write barriers where necessary.
-  new_payload.SetMarkBit();
-  payload_.store(new_payload, std::memory_order_relaxed);
+  auto old_payload = payload_.load(std::memory_order_relaxed);
+  while (true) {
+    Payload new_payload(value, tag);
+    if (old_payload.HasMarkBitSet()) {
+      new_payload.SetMarkBit();
+    }
+    if (payload_.compare_exchange_weak(old_payload, new_payload,
+                                       std::memory_order_relaxed)) {
+      break;
+    }
+  }
   MaybeUpdateRawPointerForLSan(value);
 }
 
@@ -65,15 +71,19 @@
   // The 2nd most significant byte must be empty as we store the tag in int.
   DCHECK_EQ(0, value & kExternalPointerTagAndMarkbitMask);
 
-  Payload new_payload(value, tag);
-  // Writing an entry currently also marks it as alive. In the future, we might
-  // want to drop this and instead use write barriers where necessary.
-  new_payload.SetMarkBit();
-  Payload old_payload =
-      payload_.exchange(new_payload, std::memory_order_relaxed);
-  DCHECK(old_payload.ContainsPointer());
-  MaybeUpdateRawPointerForLSan(value);
-  return old_payload.Untag(tag);
+  auto old_payload = payload_.load(std::memory_order_relaxed);
+  while (true) {
+    DCHECK(old_payload.ContainsPointer());