cppgc: Allow CrossThreadPersistent to access poisoned memory from GC

Allow CrossThreadPersistent and its weak form to access ASAN poisoned
memory from the GC entry points.

In general, payloads of to-be-finalized objects are poisoned until the
finalizer actually runs to avoid accidentally touching that payload.

In the case of cross-thread handles, these may need to be cleared by a
different thread before the finalizer actually runs. In order to clear
those references, the slot needs to be unpoisoned.

This is issue is ASAN-only and does not affect production or other
debug builds.

Bug: chromium:1230599, chromium:1056170
Change-Id: If4d0808953047319b02653821abbb5c638084dc5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3040845
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75846}

include/cppgc/cross-thread-persistent.h

@@ -13,12 +13,34 @@
 #include "cppgc/visitor.h"
 
 namespace cppgc {
-
 namespace internal {
 
+// Wrapper around PersistentBase that allows accessing poisoned memory when
+// using ASAN. This is needed as the GC of the heap that owns the value
+// of a CTP, may clear it (heap termination, weakness) while the object
+// holding the CTP may be poisoned as itself may be deemed dead.
+class CrossThreadPersistentBase : public PersistentBase {
+ public:
+  CrossThreadPersistentBase() = default;
+  explicit CrossThreadPersistentBase(const void* raw) : PersistentBase(raw) {}
+
+  V8_CLANG_NO_SANITIZE("address") const void* GetValueFromGC() const {
+    return raw_;
+  }
+
+  V8_CLANG_NO_SANITIZE("address")
+  PersistentNode* GetNodeFromGC() const { return node_; }
+
+  V8_CLANG_NO_SANITIZE("address")
+  void ClearFromGC() const {
+    raw_ = nullptr;
+    node_ = nullptr;
+  }
+};
+
 template <typename T, typename WeaknessPolicy, typename LocationPolicy,
           typename CheckingPolicy>
-class BasicCrossThreadPersistent final : public PersistentBase,
+class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
                                          public LocationPolicy,
                                          private WeaknessPolicy,
                                          private CheckingPolicy {
@@ -38,11 +60,11 @@ class BasicCrossThreadPersistent final : public PersistentBase,
 
   BasicCrossThreadPersistent(
       SentinelPointer s, const SourceLocation& loc = SourceLocation::Current())
-      : PersistentBase(s), LocationPolicy(loc) {}
+      : CrossThreadPersistentBase(s), LocationPolicy(loc) {}
 
   BasicCrossThreadPersistent(
       T* raw, const SourceLocation& loc = SourceLocation::Current())
-      : PersistentBase(raw), LocationPolicy(loc) {
+      : CrossThreadPersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid(raw)) return;
     PersistentRegionLock guard;
     CrossThreadPersistentRegion& region = this->GetPersistentRegion(raw);
@@ -61,7 +83,7 @@ class BasicCrossThreadPersistent final : public PersistentBase,
   BasicCrossThreadPersistent(
       UnsafeCtorTag, T* raw,
       const SourceLocation& loc = SourceLocation::Current())
-      : PersistentBase(raw), LocationPolicy(loc) {
+      : CrossThreadPersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid(raw)) return;
     CrossThreadPersistentRegion& region = this->GetPersistentRegion(raw);
     SetNode(region.AllocateNode(this, &Trace));
@@ -329,12 +351,19 @@ class BasicCrossThreadPersistent final : public PersistentBase,
   }
 
   void ClearFromGC() const {
-    if (IsValid(GetValue())) {
-      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
-      PersistentBase::ClearFromGC();
+    if (IsValid(GetValueFromGC())) {
+      WeaknessPolicy::GetPersistentRegion(GetValueFromGC())
+          .FreeNode(GetNodeFromGC());
+      CrossThreadPersistentBase::ClearFromGC();
     }
   }
 
+  // See Get() for details.
+  V8_CLANG_NO_SANITIZE("cfi-unrelated-cast")
+  T* GetFromGC() const {
+    return static_cast<T*>(const_cast<void*>(GetValueFromGC()));
+  }
+
   friend class cppgc::Visitor;
 };
 

include/cppgc/internal/persistent-node.h

@@ -75,7 +75,7 @@ class PersistentNode final {
   TraceCallback trace_ = nullptr;
 };
 
-class V8_EXPORT PersistentRegion final {
+class V8_EXPORT PersistentRegion {
   using PersistentNodeSlots = std::array<PersistentNode, 256u>;
 
  public:
@@ -116,6 +116,9 @@ class V8_EXPORT PersistentRegion final {
  private:
   void EnsureNodeSlots();
 
+  template <typename PersistentBaseClass>
+  void ClearAllUsedNodes();
+
   std::vector<std::unique_ptr<PersistentNodeSlots>> nodes_;
   PersistentNode* free_list_head_ = nullptr;
   size_t nodes_in_use_ = 0;
@@ -135,7 +138,7 @@ class V8_EXPORT PersistentRegionLock final {
 
 // Variant of PersistentRegion that checks whether the PersistentRegionLock is
 // locked.
-class V8_EXPORT CrossThreadPersistentRegion final {
+class V8_EXPORT CrossThreadPersistentRegion final : protected PersistentRegion {
  public:
   CrossThreadPersistentRegion() = default;
   // Clears Persistent fields to avoid stale pointers after heap teardown.
@@ -147,12 +150,12 @@ class V8_EXPORT CrossThreadPersistentRegion final {
 
   V8_INLINE PersistentNode* AllocateNode(void* owner, TraceCallback trace) {
     PersistentRegionLock::AssertLocked();
-    return persistent_region_.AllocateNode(owner, trace);
+    return PersistentRegion::AllocateNode(owner, trace);
   }
 
   V8_INLINE void FreeNode(PersistentNode* node) {
     PersistentRegionLock::AssertLocked();
-    persistent_region_.FreeNode(node);
+    PersistentRegion::FreeNode(node);
   }
 
   void Trace(Visitor*);
@@ -160,9 +163,6 @@ class V8_EXPORT CrossThreadPersistentRegion final {
   size_t NodesInUse() const;
 
   void ClearAllUsedNodes();
-
- private:
-  PersistentRegion persistent_region_;
 };
 
 }  // namespace internal

include/cppgc/member.h

@@ -218,6 +218,8 @@ class BasicMember final : private MemberBase, private CheckingPolicy {
 
   void ClearFromGC() const { MemberBase::ClearFromGC(); }
 
+  T* GetFromGC() const { return Get(); }
+
   friend class cppgc::Visitor;
   template <typename U>
   friend struct cppgc::TraceTrait;

include/cppgc/persistent.h

@@ -41,7 +41,7 @@ class PersistentBase {
     node_ = nullptr;
   }
 
- private:
+ protected:
   mutable const void* raw_ = nullptr;
   mutable PersistentNode* node_ = nullptr;
 
@@ -259,6 +259,12 @@ class BasicPersistent final : public PersistentBase,
     }
   }
 
+  // Set Get() for details.
+  V8_CLANG_NO_SANITIZE("cfi-unrelated-cast")
+  T* GetFromGC() const {
+    return static_cast<T*>(const_cast<void*>(GetValue()));
+  }
+
   friend class cppgc::Visitor;
 };
 

include/cppgc/visitor.h

@@ -12,6 +12,7 @@
 #include "cppgc/internal/pointer-policies.h"
 #include "cppgc/liveness-broker.h"
 #include "cppgc/member.h"
+#include "cppgc/sentinel-pointer.h"
 #include "cppgc/source-location.h"
 #include "cppgc/trace-trait.h"
 #include "cppgc/type-traits.h"
@@ -318,10 +319,10 @@ class V8_EXPORT Visitor {
   template <typename PointerType>
   static void HandleWeak(const LivenessBroker& info, const void* object) {
     const PointerType* weak = static_cast<const PointerType*>(object);
+    auto* raw_ptr = weak->GetFromGC();
     // Sentinel values are preserved for weak pointers.
-    if (*weak == kSentinelPointer) return;
-    const auto* raw = weak->Get();
-    if (!info.IsHeapObjectAlive(raw)) {
+    if (raw_ptr == kSentinelPointer) return;
+    if (!info.IsHeapObjectAlive(raw_ptr)) {
       weak->ClearFromGC();
     }
   }
@@ -335,11 +336,11 @@ class V8_EXPORT Visitor {
     static_assert(internal::IsGarbageCollectedOrMixinType<PointeeType>::value,
                   "Persistent's pointee type must be GarbageCollected or "
                   "GarbageCollectedMixin");
-    if (!p.Get()) {
+    auto* ptr = p.GetFromGC();
+    if (!ptr) {
       return;
     }
-    VisitRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
-              loc);
+    VisitRoot(ptr, TraceTrait<PointeeType>::GetTraceDescriptor(ptr), loc);
   }
 
   template <
@@ -354,7 +355,8 @@ class V8_EXPORT Visitor {
                   "GarbageCollectedMixin");
     static_assert(!internal::IsAllocatedOnCompactableSpace<PointeeType>::value,
                   "Weak references to compactable objects are not allowed");
-    VisitWeakRoot(p.Get(), TraceTrait<PointeeType>::GetTraceDescriptor(p.Get()),
+    auto* ptr = p.GetFromGC();
+    VisitWeakRoot(ptr, TraceTrait<PointeeType>::GetTraceDescriptor(ptr),
                   &HandleWeak<WeakPersistent>, &p, loc);
   }
 

src/heap/cppgc/persistent-node.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <numeric>
 
+#include "include/cppgc/cross-thread-persistent.h"
 #include "include/cppgc/persistent.h"
 #include "src/heap/cppgc/process-heap.h"
 
@@ -15,23 +16,32 @@ namespace internal {
 
 PersistentRegion::~PersistentRegion() { ClearAllUsedNodes(); }
 
+template <typename PersistentBaseClass>
 void PersistentRegion::ClearAllUsedNodes() {
   for (auto& slots : nodes_) {
     for (auto& node : *slots) {
-      if (node.IsUsed()) {
-        static_cast<PersistentBase*>(node.owner())->ClearFromGC();
-        // Add nodes back to the free list to allow reusing for subsequent
-        // creation calls.
-        node.InitializeAsFreeNode(free_list_head_);
-        free_list_head_ = &node;
-        CPPGC_DCHECK(nodes_in_use_ > 0);
-        nodes_in_use_--;
-      }
+      if (!node.IsUsed()) continue;
+
+      static_cast<PersistentBaseClass*>(node.owner())->ClearFromGC();
+
+      // Add nodes back to the free list to allow reusing for subsequent
+      // creation calls.
+      node.InitializeAsFreeNode(free_list_head_);
+      free_list_head_ = &node;
+      CPPGC_DCHECK(nodes_in_use_ > 0);
+      nodes_in_use_--;
     }
   }
   CPPGC_DCHECK(0u == nodes_in_use_);
 }
 
+template void PersistentRegion::ClearAllUsedNodes<CrossThreadPersistentBase>();
+template void PersistentRegion::ClearAllUsedNodes<PersistentBase>();
+
+void PersistentRegion::ClearAllUsedNodes() {
+  ClearAllUsedNodes<PersistentBase>();
+}
+
 size_t PersistentRegion::NodesInUse() const {
 #ifdef DEBUG
   const size_t accumulated_nodes_in_use_ = std::accumulate(
@@ -97,23 +107,24 @@ void PersistentRegionLock::AssertLocked() {
 
 CrossThreadPersistentRegion::~CrossThreadPersistentRegion() {
   PersistentRegionLock guard;
-  persistent_region_.ClearAllUsedNodes();
-  persistent_region_.nodes_.clear();
+  PersistentRegion::ClearAllUsedNodes<CrossThreadPersistentBase>();
+  nodes_.clear();
+  // PersistentRegion destructor will be a noop.
 }
 
 void CrossThreadPersistentRegion::Trace(Visitor* visitor) {
   PersistentRegionLock::AssertLocked();
-  return persistent_region_.Trace(visitor);
+  PersistentRegion::Trace(visitor);
 }
 
 size_t CrossThreadPersistentRegion::NodesInUse() const {
   // This method does not require a lock.
-  return persistent_region_.NodesInUse();
+  return PersistentRegion::NodesInUse();
 }
 
 void CrossThreadPersistentRegion::ClearAllUsedNodes() {
   PersistentRegionLock::AssertLocked();
-  return persistent_region_.ClearAllUsedNodes();
+  PersistentRegion::ClearAllUsedNodes<CrossThreadPersistentBase>();
 }
 
 }  // namespace internal

src/heap/cppgc/process-heap.h

@@ -32,6 +32,7 @@ class V8_EXPORT_PRIVATE HeapRegistry final {
 
   static HeapBase* TryFromManagedPointer(const void* needle);
 
+  // Does not take the registry mutex and is thus only useful for testing.
   static const Storage& GetRegisteredHeapsForTesting();
 
  private:

src/heap/cppgc/sweeper.cc

@@ -599,8 +599,10 @@ class ConcurrentSweepTask final : public cppgc::JobTask,
 // This visitor:
 // - clears free lists for all spaces;
 // - moves all Heap pages to local Sweeper's state (SpaceStates).
+// - ASAN: Poisons all unmarked object payloads.
 class PrepareForSweepVisitor final
-    : public HeapVisitor<PrepareForSweepVisitor> {
+    : protected HeapVisitor<PrepareForSweepVisitor> {
+  friend class HeapVisitor<PrepareForSweepVisitor>;
   using CompactableSpaceHandling =
       Sweeper::SweepingConfig::CompactableSpaceHandling;
 
@@ -610,6 +612,9 @@ class PrepareForSweepVisitor final
       : states_(states),
         compactable_space_handling_(compactable_space_handling) {}
 
+  void Run(RawHeap& raw_heap) { Traverse(raw_heap); }
+
+ protected:
   bool VisitNormalPageSpace(NormalPageSpace& space) {
     if ((compactable_space_handling_ == CompactableSpaceHandling::kIgnore) &&
         space.is_compactable())
@@ -679,7 +684,7 @@ class Sweeper::SweeperImpl final {
     }
 
     PrepareForSweepVisitor(&space_states_, config.compactable_space_handling)
-        .Traverse(heap_);
+        .Run(heap_);
 
     if (config.sweeping_type == SweepingConfig::SweepingType::kAtomic) {
       Finish();

test/unittests/heap/cppgc/sweeper-unittest.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "include/cppgc/allocation.h"
+#include "include/cppgc/cross-thread-persistent.h"
 #include "include/cppgc/persistent.h"
 #include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap-object-header.h"
@@ -303,7 +304,7 @@ TEST_F(SweeperTest, LazySweepingDuringAllocation) {
       Heap::Config::MarkingType::kAtomic,
       Heap::Config::SweepingType::kIncrementalAndConcurrent};
   Heap::From(GetHeap())->CollectGarbage(config);
-  // Incremetal sweeping is active and the space should have two pages with
+  // Incremental sweeping is active and the space should have two pages with
   // no room for an additional GCedObject. Allocating a new GCedObject should
   // trigger sweeping. All objects other than the 2nd object on each page are
   // marked. Lazy sweeping on allocation should reclaim the object on one of
@@ -400,5 +401,94 @@ TEST_F(SweeperTest, DiscardingNormalPageMemory) {
   USE(holder);
 }
 
+namespace {
+
+class Holder final : public GarbageCollected<Holder> {
+ public:
+  static size_t destructor_callcount;
+
+  void Trace(Visitor*) const {}
+
+  ~Holder() {
+    EXPECT_FALSE(ref);
+    EXPECT_FALSE(weak_ref);
+    destructor_callcount++;
+  }
+
+  cppgc::subtle::CrossThreadPersistent<GCed<1>> ref;
+  cppgc::subtle::WeakCrossThreadPersistent<GCed<1>> weak_ref;
+};
+
+// static
+size_t Holder::destructor_callcount;
+
+}  // namespace
+
+TEST_F(SweeperTest, CrossThreadPersistentCanBeClearedFromOtherThread) {
+  Holder::destructor_callcount = 0;
+  auto* holder = MakeGarbageCollected<Holder>(GetAllocationHandle());
+
+  auto remote_heap = cppgc::Heap::Create(GetPlatformHandle());
+  // The case below must be able to clear both, the CTP and WCTP.
+  holder->ref =
+      MakeGarbageCollected<GCed<1>>(remote_heap->GetAllocationHandle());
+  holder->weak_ref =
+      MakeGarbageCollected<GCed<1>>(remote_heap->GetAllocationHandle());
+
+  testing::TestPlatform::DisableBackgroundTasksScope no_concurrent_sweep_scope(
+      GetPlatformHandle().get());
+  Heap::From(GetHeap())->CollectGarbage(
+      {Heap::Config::CollectionType::kMajor,
+       Heap::Config::StackState::kNoHeapPointers,
+       Heap::Config::MarkingType::kAtomic,
+       Heap::Config::SweepingType::kIncrementalAndConcurrent});
+  // `holder` is unreachable (as the stack is not scanned) and will be
+  // reclaimed. Its payload memory is generally poisoned at this point. The
+  // CrossThreadPersistent slot should be unpoisoned.
+
+  // Terminate the remote heap which should also clear `holder->ref`. The slot
+  // for `ref` should have been unpoisoned by the GC.
+  Heap::From(remote_heap.get())->Terminate();
+
+  // Finish the sweeper which will find the CrossThreadPersistent in cleared
+  // state.
+  Heap::From(GetHeap())->sweeper().FinishIfRunning();
+  EXPECT_EQ(1u, Holder::destructor_callcount);
+}
+
+TEST_F(SweeperTest, WeakCrossThreadPersistentCanBeClearedFromOtherThread) {
+  Holder::destructor_callcount = 0;
+  auto* holder = MakeGarbageCollected<Holder>(GetAllocationHandle());
+
+  auto remote_heap = cppgc::Heap::Create(GetPlatformHandle());
+  holder->weak_ref =
+      MakeGarbageCollected<GCed<1>>(remote_heap->GetAllocationHandle());
+
+  testing::TestPlatform::DisableBackgroundTasksScope no_concurrent_sweep_scope(
+      GetPlatformHandle().get());
+  static constexpr Heap::Config config = {
+      Heap::Config::CollectionType::kMajor,
+      Heap::Config::StackState::kNoHeapPointers,
+      Heap::Config::MarkingType::kAtomic,
+      Heap::Config::SweepingType::kIncrementalAndConcurrent};
+  Heap::From(GetHeap())->CollectGarbage(config);
+  // `holder` is unreachable (as the stack is not scanned) and will be
+  // reclaimed. Its payload memory is generally poisoned at this point. The
+  // WeakCrossThreadPersistent slot should be unpoisoned during clearing.
+
+  // GC in the remote heap should also clear `holder->weak_ref`. The slot for
+  // `weak_ref` should be unpoisoned by the GC.
+  Heap::From(remote_heap.get())
+      ->CollectGarbage({Heap::Config::CollectionType::kMajor,
+                        Heap::Config::StackState::kNoHeapPointers,
+                        Heap::Config::MarkingType::kAtomic,
+                        Heap::Config::SweepingType::kAtomic});
+
+  // Finish the sweeper which will find the CrossThreadPersistent in cleared
+  // state.
+  Heap::From(GetHeap())->sweeper().FinishIfRunning();
+  EXPECT_EQ(1u, Holder::destructor_callcount);
+}
+
 }  // namespace internal
 }  // namespace cppgc