Reland "cppgc: Properly clear (Weak)Peristent and WeakMember pointers"

This is a reland of e0c1a349

The issue was passing SentinelPointer (== +1) through T*.

The fix is disabling cfi unrelated cast diagnostic for the bottlenecks
(Get()). This means that nullptr is treated the same as
kSentinelPointer.

The alternative would be a DCHECK that Get() does not return
kSentinelPointer and adjusting all Member and Persistent logic that
uses Get() to work on void*. This is quite intrusive as it involves
Swap(), heterogeneous assignments, comparisons, etc.

Original change's description:
> cppgc: Properly clear (Weak)Peristent and WeakMember pointers
>
> The CL addresses two issues with (Weak)Persistent and WeakMember:
> 1. (Weak)Persistent pointers are cleared on heap teardown. Before this
>    CL the pointers would contain stale values which could lead to UAF.
> 2. WeakPersistent and WeakMember are cleared using a combination of
>    internal clearing methods and mutable fields which avoids the use
>    of const_cast<>.
>
> Bug: chromium:1056170
> Change-Id: Ibf2b0f0856771b4f6906608cde13a6d43ebf81f3
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2248190
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Reviewed-by: Anton Bikineev <bikineev@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#68394}

Bug: chromium:1056170
Change-Id: I3d74b43464c2973df1956f51b1419d755dd9f519
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2250240Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68426}

include/cppgc/internal/persistent-node.h

@@ -56,6 +56,11 @@ class PersistentNode final {
 
   bool IsUsed() const { return trace_; }
 
+  void* owner() const {
+    CPPGC_DCHECK(IsUsed());
+    return owner_;
+  }
+
  private:
   // PersistentNode acts as a designated union:
   // If trace_ != nullptr, owner_ points to the corresponding Persistent handle.
@@ -67,11 +72,13 @@ class PersistentNode final {
   TraceCallback trace_ = nullptr;
 };
 
-class V8_EXPORT PersistentRegion {
+class V8_EXPORT PersistentRegion final {
   using PersistentNodeSlots = std::array<PersistentNode, 256u>;
 
  public:
   PersistentRegion() = default;
+  // Clears Persistent fields to avoid stale pointers after heap teardown.
+  ~PersistentRegion();
 
   PersistentRegion(const PersistentRegion&) = delete;
   PersistentRegion& operator=(const PersistentRegion&) = delete;

include/cppgc/liveness-broker.h

@@ -49,12 +49,6 @@ class V8_EXPORT LivenessBroker final {
                TraceTrait<T>::GetTraceDescriptor(object).base_object_payload);
   }
 
-  template <typename T>
-  bool IsHeapObjectAlive(const WeakMember<T>& weak_member) const {
-    return (weak_member != kSentinelPointer) &&
-           IsHeapObjectAlive<T>(weak_member.Get());
-  }
-
   template <typename T>
   bool IsHeapObjectAlive(const UntracedMember<T>& untraced_member) const {
     return (untraced_member != kSentinelPointer) &&

include/cppgc/member.h

@@ -19,19 +19,43 @@ class Visitor;
 
 namespace internal {
 
+class MemberBase {
+ protected:
+  MemberBase() = default;
+  explicit MemberBase(void* value) : raw_(value) {}
+
+  void* const* GetRawSlot() const { return &raw_; }
+  void* GetRaw() const { return raw_; }
+  void SetRaw(void* value) { raw_ = value; }
+
+  void* GetRawAtomic() const {
+    return reinterpret_cast<const std::atomic<void*>*>(&raw_)->load(
+        std::memory_order_relaxed);
+  }
+  void SetRawAtomic(void* value) {
+    reinterpret_cast<std::atomic<void*>*>(&raw_)->store(
+        value, std::memory_order_relaxed);
+  }
+
+  void ClearFromGC() const { raw_ = nullptr; }
+
+ private:
+  mutable void* raw_ = nullptr;
+};
+
 // The basic class from which all Member classes are 'generated'.
 template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
           typename CheckingPolicy>
-class BasicMember : private CheckingPolicy {
+class BasicMember final : private MemberBase, private CheckingPolicy {
  public:
   using PointeeType = T;
 
   constexpr BasicMember() = default;
   constexpr BasicMember(std::nullptr_t) {}     // NOLINT
-  BasicMember(SentinelPointer s) : raw_(s) {}  // NOLINT
-  BasicMember(T* raw) : raw_(raw) {            // NOLINT
+  BasicMember(SentinelPointer s) : MemberBase(s) {}  // NOLINT
+  BasicMember(T* raw) : MemberBase(raw) {            // NOLINT
     InitializingWriteBarrier();
-    this->CheckPointer(raw_);
+    this->CheckPointer(Get());
   }
   BasicMember(T& raw) : BasicMember(&raw) {}  // NOLINT
   BasicMember(const BasicMember& other) : BasicMember(other.Get()) {}
@@ -106,9 +130,12 @@ class BasicMember : private CheckingPolicy {
   T* operator->() const { return Get(); }
   T& operator*() const { return *Get(); }
 
-  T* Get() const {
+  // CFI cast exemption to allow passing SentinelPointer through T* and support
+  // heterogeneous assignments between different Member and Persistent handles
+  // based on their actual types.
+  V8_CLANG_NO_SANITIZE("cfi-unrelated-cast") T* Get() const {
     // Executed by the mutator, hence non atomic load.
-    return raw_;
+    return static_cast<T*>(MemberBase::GetRaw());
   }
 
   void Clear() { SetRawAtomic(nullptr); }
@@ -120,25 +147,18 @@ class BasicMember : private CheckingPolicy {
   }
 
  private:
-  void SetRawAtomic(T* raw) {
-    reinterpret_cast<std::atomic<T*>*>(&raw_)->store(raw,
-                                                     std::memory_order_relaxed);
-  }
   T* GetRawAtomic() const {
-    return reinterpret_cast<const std::atomic<T*>*>(&raw_)->load(
-        std::memory_order_relaxed);
+    return static_cast<T*>(MemberBase::GetRawAtomic());
   }
 
   void InitializingWriteBarrier() const {
-    WriteBarrierPolicy::InitializingBarrier(
-        reinterpret_cast<const void*>(&raw_), static_cast<const void*>(raw_));
+    WriteBarrierPolicy::InitializingBarrier(GetRawSlot(), GetRaw());
   }
   void AssigningWriteBarrier() const {
-    WriteBarrierPolicy::AssigningBarrier(reinterpret_cast<const void*>(&raw_),
-                                         static_cast<const void*>(raw_));
+    WriteBarrierPolicy::AssigningBarrier(GetRawSlot(), GetRaw());
   }
 
-  T* raw_ = nullptr;
+  void ClearFromGC() const { MemberBase::ClearFromGC(); }
 
   friend class cppgc::Visitor;
 };

include/cppgc/persistent.h

@@ -15,12 +15,41 @@
 #include "v8config.h"  // NOLINT(build/include_directory)
 
 namespace cppgc {
+
+class Visitor;
+
 namespace internal {
 
+class PersistentBase {
+ protected:
+  PersistentBase() = default;
+  explicit PersistentBase(void* raw) : raw_(raw) {}
+
+  void* GetValue() const { return raw_; }
+  void SetValue(void* value) { raw_ = value; }
+
+  PersistentNode* GetNode() const { return node_; }
+  void SetNode(PersistentNode* node) { node_ = node; }
+
+  // Performs a shallow clear which assumes that internal persistent nodes are
+  // destroyed elsewhere.
+  void ClearFromGC() const {
+    raw_ = nullptr;
+    node_ = nullptr;
+  }
+
+ private:
+  mutable void* raw_ = nullptr;
+  mutable PersistentNode* node_ = nullptr;
+
+  friend class PersistentRegion;
+};
+
 // The basic class from which all Persistent classes are generated.
 template <typename T, typename WeaknessPolicy, typename LocationPolicy,
           typename CheckingPolicy>
-class BasicPersistent : public LocationPolicy,
+class BasicPersistent final : public PersistentBase,
+                              public LocationPolicy,
                               private WeaknessPolicy,
                               private CheckingPolicy {
  public:
@@ -38,15 +67,15 @@ class BasicPersistent : public LocationPolicy,
 
   BasicPersistent(  // NOLINT
       SentinelPointer s, const SourceLocation& loc = SourceLocation::Current())
-      : LocationPolicy(loc), raw_(s) {}
+      : PersistentBase(s), LocationPolicy(loc) {}
 
-  // Raw value contstructors.
+  // Raw value constructors.
   BasicPersistent(T* raw,  // NOLINT
                   const SourceLocation& loc = SourceLocation::Current())
-      : LocationPolicy(loc), raw_(raw) {
+      : PersistentBase(raw), LocationPolicy(loc) {
     if (!IsValid()) return;
-    node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode(
-        this, &BasicPersistent::Trace);
+    SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
+                .AllocateNode(this, &BasicPersistent::Trace));
     this->CheckPointer(Get());
   }
 
@@ -74,13 +103,11 @@ class BasicPersistent : public LocationPolicy,
   BasicPersistent(
       BasicPersistent&& other,
       const SourceLocation& loc = SourceLocation::Current()) noexcept
-      : LocationPolicy(std::move(other)),
-        raw_(std::move(other.raw_)),
-        node_(std::move(other.node_)) {
+      : PersistentBase(std::move(other)), LocationPolicy(std::move(other)) {
     if (!IsValid()) return;
-    node_->UpdateOwner(this);
-    other.raw_ = nullptr;
-    other.node_ = nullptr;
+    GetNode()->UpdateOwner(this);
+    other.SetValue(nullptr);
+    other.SetNode(nullptr);
     this->CheckPointer(Get());
   }
 
@@ -114,13 +141,12 @@ class BasicPersistent : public LocationPolicy,
   BasicPersistent& operator=(BasicPersistent&& other) {
     if (this == &other) return *this;
     Clear();
+    PersistentBase::operator=(std::move(other));
     LocationPolicy::operator=(std::move(other));
-    raw_ = std::move(other.raw_);
-    node_ = std::move(other.node_);
     if (!IsValid()) return *this;
-    node_->UpdateOwner(this);
-    other.raw_ = nullptr;
-    other.node_ = nullptr;
+    GetNode()->UpdateOwner(this);
+    other.SetValue(nullptr);
+    other.SetNode(nullptr);
     this->CheckPointer(Get());
     return *this;
   }
@@ -156,7 +182,12 @@ class BasicPersistent : public LocationPolicy,
   T* operator->() const { return Get(); }
   T& operator*() const { return *Get(); }
 
-  T* Get() const { return raw_; }
+  // CFI cast exemption to allow passing SentinelPointer through T* and support
+  // heterogeneous assignments between different Member and Persistent handles
+  // based on their actual types.
+  V8_CLANG_NO_SANITIZE("cfi-unrelated-cast") T* Get() const {
+    return static_cast<T*>(GetValue());
+  }
 
   void Clear() { Assign(nullptr); }
 
@@ -176,29 +207,35 @@ class BasicPersistent : public LocationPolicy,
     // Ideally, handling kSentinelPointer would be done by the embedder. On the
     // other hand, having Persistent aware of it is beneficial since no node
     // gets wasted.
-    return raw_ != nullptr && raw_ != kSentinelPointer;
+    return GetValue() != nullptr && GetValue() != kSentinelPointer;
   }
 
   void Assign(T* ptr) {
     if (IsValid()) {
       if (ptr && ptr != kSentinelPointer) {
         // Simply assign the pointer reusing the existing node.
-        raw_ = ptr;
+        SetValue(ptr);
         this->CheckPointer(ptr);
         return;
       }
-      WeaknessPolicy::GetPersistentRegion(raw_).FreeNode(node_);
-      node_ = nullptr;
+      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
+      SetNode(nullptr);
     }
-    raw_ = ptr;
+    SetValue(ptr);
     if (!IsValid()) return;
-    node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode(
-        this, &BasicPersistent::Trace);
+    SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
+                .AllocateNode(this, &BasicPersistent::Trace));
     this->CheckPointer(Get());
   }
 
-  T* raw_ = nullptr;
-  PersistentNode* node_ = nullptr;
+  void ClearFromGC() const {
+    if (IsValid()) {
+      WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
+      PersistentBase::ClearFromGC();
+    }
+  }
+
+  friend class cppgc::Visitor;
 };
 
 template <typename T1, typename WeaknessPolicy1, typename LocationPolicy1,

include/cppgc/visitor.h

@@ -136,12 +136,11 @@ class Visitor {
   template <typename PointerType>
   static void HandleWeak(const LivenessBroker& info, const void* object) {
     const PointerType* weak = static_cast<const PointerType*>(object);
+    // Sentinel values are preserved for weak pointers.
+    if (*weak == kSentinelPointer) return;
     const auto* raw = weak->Get();
-    if (raw && !info.IsHeapObjectAlive(raw)) {
-      // Object is passed down through the marker as const. Alternatives are
-      // - non-const Trace method;
-      // - mutable pointer in MemberBase;
-      const_cast<PointerType*>(weak)->Clear();
+    if (!info.IsHeapObjectAlive(raw)) {
+      weak->ClearFromGC();
     }
   }
 

include/v8config.h

@@ -433,6 +433,16 @@
 #define V8_WARN_UNUSED_RESULT /* NOT SUPPORTED */
 #endif
 
+// Helper macro to define no_sanitize attributes only with clang.
+#if defined(__clang__) && defined(__has_attribute)
+#if __has_attribute(no_sanitize)
+#define V8_CLANG_NO_SANITIZE(what) __attribute__((no_sanitize(what)))
+#endif
+#endif
+#if !defined(V8_CLANG_NO_SANITIZE)
+#define V8_CLANG_NO_SANITIZE(what)
+#endif
+
 #if defined(BUILDING_V8_SHARED) && defined(USING_V8_SHARED)
 #error Inconsistent build configuration: To build the V8 shared library \
 set BUILDING_V8_SHARED, to include its headers for linking against the \

src/base/macros.h

@@ -171,22 +171,12 @@ V8_INLINE Dest bit_cast(Source const& source) {
 #endif
 #endif
 
-// Helper macro to define no_sanitize attributes only with clang.
-#if defined(__clang__) && defined(__has_attribute)
-#if __has_attribute(no_sanitize)
-#define CLANG_NO_SANITIZE(what) __attribute__((no_sanitize(what)))
-#endif
-#endif
-#if !defined(CLANG_NO_SANITIZE)
-#define CLANG_NO_SANITIZE(what)
-#endif
-
 // DISABLE_CFI_PERF -- Disable Control Flow Integrity checks for Perf reasons.
-#define DISABLE_CFI_PERF CLANG_NO_SANITIZE("cfi")
+#define DISABLE_CFI_PERF V8_CLANG_NO_SANITIZE("cfi")
 
 // DISABLE_CFI_ICALL -- Disable Control Flow Integrity indirect call checks,
 // useful because calls into JITed code can not be CFI verified.
-#define DISABLE_CFI_ICALL CLANG_NO_SANITIZE("cfi-icall")
+#define DISABLE_CFI_ICALL V8_CLANG_NO_SANITIZE("cfi-icall")
 
 #if V8_CC_GNU
 #define V8_IMMEDIATE_CRASH() __builtin_trap()

src/heap/cppgc/persistent-node.cc

@@ -7,9 +7,21 @@
 #include <algorithm>
 #include <numeric>
 
+#include "include/cppgc/persistent.h"
+
 namespace cppgc {
 namespace internal {
 
+PersistentRegion::~PersistentRegion() {
+  for (auto& slots : nodes_) {
+    for (auto& node : *slots) {
+      if (node.IsUsed()) {
+        static_cast<PersistentBase*>(node.owner())->ClearFromGC();
+      }
+    }
+  }
+}
+
 size_t PersistentRegion::NodesInUse() const {
   return std::accumulate(
       nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) {

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

@@ -5,6 +5,7 @@
 #include "src/heap/cppgc/marker.h"
 
 #include "include/cppgc/allocation.h"
+#include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/persistent.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
@@ -245,5 +246,20 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
       });
 }
 
+TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) {
+  Marker marker(Heap::From(GetHeap())->AsBase());
+  Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
+  auto* tmp = MakeGarbageCollected<GCed>(GetAllocationHandle());
+  root->SetWeakChild(tmp);
+  static const Marker::MarkingConfig config = {
+      MarkingConfig::CollectionType::kMajor,
+      MarkingConfig::StackState::kNoHeapPointers};
+  marker.StartMarking(config);
+  marker.FinishMarking(config);
+  root->SetWeakChild(kSentinelPointer);
+  marker.ProcessWeakness();
+  EXPECT_EQ(kSentinelPointer, root->weak_child());
+}
+
 }  // namespace internal
 }  // namespace cppgc

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

@@ -8,6 +8,7 @@
 
 #include "include/cppgc/allocation.h"
 #include "include/cppgc/garbage-collected.h"
+#include "include/cppgc/internal/pointer-policies.h"
 #include "include/cppgc/member.h"
 #include "include/cppgc/type-traits.h"
 #include "src/heap/cppgc/heap.h"
@@ -70,6 +71,7 @@ class RootVisitor final : public VisitorBase {
  private:
   std::vector<std::pair<WeakCallback, const void*>> weak_callbacks_;
 };
+
 class PersistentTest : public testing::TestSupportingAllocationOnly {};
 
 }  // namespace
@@ -617,6 +619,25 @@ TEST_F(PersistentTest, TraceWeak) {
   EXPECT_EQ(0u, GetRegion<WeakPersistent>(heap).NodesInUse());
 }
 
+TEST_F(PersistentTest, ClearOnHeapDestruction) {
+  Persistent<GCed> persistent;
+  WeakPersistent<GCed> weak_persistent;
+
+  // Create another heap that can be destroyed during the test.
+  auto heap = Heap::Create(GetPlatformHandle());
+  persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
+  weak_persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
+  const Persistent<GCed> persistent_sentinel(kSentinelPointer);
+  const WeakPersistent<GCed> weak_persistent_sentinel(kSentinelPointer);
+  heap.reset();
+
+  EXPECT_EQ(nullptr, persistent);
+  EXPECT_EQ(nullptr, weak_persistent);
+  // Sentinel values survive as they do not represent actual heap objects.
+  EXPECT_EQ(kSentinelPointer, persistent_sentinel);
+  EXPECT_EQ(kSentinelPointer, weak_persistent_sentinel);
+}
+
 #if CPPGC_SUPPORTS_SOURCE_LOCATION
 TEST_F(PersistentTest, LocalizedPersistent) {
   GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());

test/unittests/heap/cppgc/tests.h

@@ -22,6 +22,8 @@ class TestWithPlatform : public ::testing::Test {
 
   TestPlatform& GetPlatform() const { return *platform_; }
 
+  std::shared_ptr<TestPlatform> GetPlatformHandle() const { return platform_; }
+
  protected:
   static std::shared_ptr<TestPlatform> platform_;
 };