cppgc: Improve Member checking

Create verification state on first assignment and check that
the reference slot is contained within the values heap if it
is an on-heap reference.

Bug: chromium:1056170
Change-Id: I0ce0e2bbd751186429950bb4f6bad97b273b3128
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2887509
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Auto-Submit: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74607}

include/cppgc/internal/pointer-policies.h

@@ -17,6 +17,7 @@
 namespace cppgc {
 namespace internal {
 
+class HeapBase;
 class PersistentRegion;
 class CrossThreadPersistentRegion;
 
@@ -76,7 +77,7 @@ class V8_EXPORT EnabledCheckingPolicy {
     }
   };
 
-  void* state_ = nullptr;
+  const HeapBase* heap_ = nullptr;
 };
 
 class DisabledCheckingPolicy {

src/heap/cppgc/heap-base.h

@@ -19,6 +19,7 @@
 #include "src/heap/cppgc/metric-recorder.h"
 #include "src/heap/cppgc/object-allocator.h"
 #include "src/heap/cppgc/process-heap-statistics.h"
+#include "src/heap/cppgc/process-heap.h"
 #include "src/heap/cppgc/raw-heap.h"
 #include "src/heap/cppgc/sweeper.h"
 #include "v8config.h"  // NOLINT(build/include_directory)
@@ -212,6 +213,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle {
   std::unique_ptr<v8::base::LsanPageAllocator> lsan_page_allocator_;
 #endif  // LEAK_SANITIZER
 
+  HeapRegistry::Subscription heap_registry_subscription_{*this};
+
 #if defined(CPPGC_CAGED_HEAP)
   CagedHeap caged_heap_;
 #endif  // CPPGC_CAGED_HEAP

src/heap/cppgc/pointer-policies.cc

@@ -12,6 +12,8 @@
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-page.h"
 #include "src/heap/cppgc/heap.h"
+#include "src/heap/cppgc/page-memory.h"
+#include "src/heap/cppgc/process-heap.h"
 
 namespace cppgc {
 namespace internal {
@@ -36,28 +38,26 @@ void EnabledCheckingPolicy::CheckPointerImpl(const void* ptr,
   // valid for large objects.
   DCHECK_IMPLIES(base_page->is_large(), points_to_payload);
 
-  if (!state_) {
-    state_ = base_page->heap();
-    // Member references are used from within objects that cannot change their
-    // heap association which means that state is immutable once it is set.
-    //
-    // TODO(chromium:1056170): Binding state late allows for getting the initial
-    // state wrong which requires a check that `this` is contained in heap that
-    // is itself expensive. Investigate options on non-caged builds to improve
-    // coverage.
+  // References cannot change their heap association which means that state is
+  // immutable once it is set.
+  if (!heap_) {
+    heap_ = base_page->heap();
+    if (!heap_->page_backend()->Lookup(reinterpret_cast<Address>(this))) {
+      // If `this` is not contained within the heap of `ptr`, we must deal with
+      // an on-stack or off-heap reference. For both cases there should be no
+      // heap registered.
+      CHECK(!HeapRegistry::TryFromManagedPointer(this));
+    }
   }
 
-  HeapBase* heap = static_cast<HeapBase*>(state_);
-  if (!heap) return;
-
   // Member references should never mix heaps.
-  DCHECK_EQ(heap, base_page->heap());
+  DCHECK_EQ(heap_, base_page->heap());
 
   // Header checks.
   const HeapObjectHeader* header = nullptr;
   if (points_to_payload) {
     header = &HeapObjectHeader::FromObject(ptr);
-  } else if (!heap->sweeper().IsSweepingInProgress()) {
+  } else if (!heap_->sweeper().IsSweepingInProgress()) {
     // Mixin case.
     header = &base_page->ObjectHeaderFromInnerAddress(ptr);
     DCHECK_LE(header->ObjectStart(), ptr);

src/heap/cppgc/process-heap.cc

@@ -4,10 +4,64 @@
 
 #include "src/heap/cppgc/process-heap.h"
 
+#include <algorithm>
+#include <vector>
+
+#include "src/base/lazy-instance.h"
+#include "src/base/platform/mutex.h"
+#include "src/heap/cppgc/heap-base.h"
+#include "src/heap/cppgc/page-memory.h"
+
 namespace cppgc {
 namespace internal {
 
 v8::base::LazyMutex g_process_mutex = LAZY_MUTEX_INITIALIZER;
 
+namespace {
+
+HeapRegistry::Storage& GetHeapRegistryStorage() {
+  static v8::base::LazyInstance<HeapRegistry::Storage>::type heap_registry =
+      LAZY_INSTANCE_INITIALIZER;
+  return *heap_registry.Pointer();
+}
+
+}  // namespace
+
+// static
+void HeapRegistry::RegisterHeap(HeapBase& heap) {
+  v8::base::MutexGuard guard(g_process_mutex.Pointer());
+
+  auto& storage = GetHeapRegistryStorage();
+  DCHECK_EQ(storage.end(), std::find(storage.begin(), storage.end(), &heap));
+  storage.push_back(&heap);
+}
+
+// static
+void HeapRegistry::UnregisterHeap(HeapBase& heap) {
+  v8::base::MutexGuard guard(g_process_mutex.Pointer());
+
+  auto& storage = GetHeapRegistryStorage();
+  const auto pos = std::find(storage.begin(), storage.end(), &heap);
+  DCHECK_NE(storage.end(), pos);
+  storage.erase(pos);
+}
+
+// static
+HeapBase* HeapRegistry::TryFromManagedPointer(const void* needle) {
+  v8::base::MutexGuard guard(g_process_mutex.Pointer());
+
+  for (auto* heap : GetHeapRegistryStorage()) {
+    const auto address =
+        heap->page_backend()->Lookup(reinterpret_cast<ConstAddress>(needle));
+    if (address) return heap;
+  }
+  return nullptr;
+}
+
+// static
+const HeapRegistry::Storage& HeapRegistry::GetRegisteredHeapsForTesting() {
+  return GetHeapRegistryStorage();
+}
+
 }  // namespace internal
 }  // namespace cppgc

src/heap/cppgc/process-heap.h

@@ -5,13 +5,48 @@
 #ifndef V8_HEAP_CPPGC_PROCESS_HEAP_H_
 #define V8_HEAP_CPPGC_PROCESS_HEAP_H_
 
+#include <vector>
+
+#include "src/base/macros.h"
 #include "src/base/platform/mutex.h"
 
 namespace cppgc {
 namespace internal {
 
+class HeapBase;
+
 extern v8::base::LazyMutex g_process_mutex;
 
+class V8_EXPORT_PRIVATE HeapRegistry final {
+ public:
+  using Storage = std::vector<HeapBase*>;
+
+  class Subscription final {
+   public:
+    inline explicit Subscription(HeapBase&);
+    inline ~Subscription();
+
+   private:
+    HeapBase& heap_;
+  };
+
+  static HeapBase* TryFromManagedPointer(const void* needle);
+
+  static const Storage& GetRegisteredHeapsForTesting();
+
+ private:
+  static void RegisterHeap(HeapBase&);
+  static void UnregisterHeap(HeapBase&);
+};
+
+HeapRegistry::Subscription::Subscription(HeapBase& heap) : heap_(heap) {
+  HeapRegistry::RegisterHeap(heap_);
+}
+
+HeapRegistry::Subscription::~Subscription() {
+  HeapRegistry::UnregisterHeap(heap_);
+}
+
 }  // namespace internal
 }  // namespace cppgc
 

test/unittests/BUILD.gn

@@ -102,6 +102,7 @@ v8_source_set("cppgc_unittests_sources") {
     "heap/cppgc/heap-growing-unittest.cc",
     "heap/cppgc/heap-object-header-unittest.cc",
     "heap/cppgc/heap-page-unittest.cc",
+    "heap/cppgc/heap-registry-unittest.cc",
     "heap/cppgc/heap-statistics-collector-unittest.cc",
     "heap/cppgc/heap-unittest.cc",
     "heap/cppgc/incremental-marking-schedule-unittest.cc",

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

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+
+#include "include/cppgc/allocation.h"
+#include "include/cppgc/heap.h"
+#include "src/heap/cppgc/heap-base.h"
+#include "src/heap/cppgc/process-heap.h"
+#include "test/unittests/heap/cppgc/tests.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace cppgc {
+namespace internal {
+
+class HeapRegistryTest : public testing::TestWithPlatform {};
+
+TEST_F(HeapRegistryTest, Empty) {
+  EXPECT_EQ(0u, HeapRegistry::GetRegisteredHeapsForTesting().size());
+}
+
+namespace {
+
+bool Contains(const HeapRegistry::Storage& storage, const cppgc::Heap* needle) {
+  return storage.end() !=
+         std::find(storage.begin(), storage.end(),
+                   &cppgc::internal::Heap::From(needle)->AsBase());
+}
+
+}  // namespace
+
+TEST_F(HeapRegistryTest, RegisterUnregisterHeaps) {
+  const auto& storage = HeapRegistry::GetRegisteredHeapsForTesting();
+  EXPECT_EQ(0u, storage.size());
+  {
+    const auto heap1 = Heap::Create(platform_);
+    EXPECT_TRUE(Contains(storage, heap1.get()));
+    EXPECT_EQ(1u, storage.size());
+    {
+      const auto heap2 = Heap::Create(platform_);
+      EXPECT_TRUE(Contains(storage, heap1.get()));
+      EXPECT_TRUE(Contains(storage, heap2.get()));
+      EXPECT_EQ(2u, storage.size());
+    }
+    EXPECT_TRUE(Contains(storage, heap1.get()));
+    EXPECT_EQ(1u, storage.size());
+  }
+  EXPECT_EQ(0u, storage.size());
+}
+
+TEST_F(HeapRegistryTest, DoesNotFindNullptr) {
+  const auto heap = Heap::Create(platform_);
+  EXPECT_EQ(nullptr, HeapRegistry::TryFromManagedPointer(nullptr));
+}
+
+TEST_F(HeapRegistryTest, DoesNotFindStackAddress) {
+  const auto heap = Heap::Create(platform_);
+  EXPECT_EQ(nullptr, HeapRegistry::TryFromManagedPointer(&heap));
+}
+
+TEST_F(HeapRegistryTest, DoesNotFindOffHeap) {
+  const auto heap = Heap::Create(platform_);
+  auto dummy = std::make_unique<char>();
+  EXPECT_EQ(nullptr, HeapRegistry::TryFromManagedPointer(dummy.get()));
+}
+
+namespace {
+
+class GCed final : public GarbageCollected<GCed> {
+ public:
+  void Trace(Visitor*) const {}
+};
+
+}  // namespace
+
+TEST_F(HeapRegistryTest, FindsRightHeapForOnHeapAddress) {
+  const auto heap1 = Heap::Create(platform_);
+  const auto heap2 = Heap::Create(platform_);
+  auto* o = MakeGarbageCollected<GCed>(heap1->GetAllocationHandle());
+  EXPECT_EQ(&cppgc::internal::Heap::From(heap1.get())->AsBase(),
+            HeapRegistry::TryFromManagedPointer(o));
+  EXPECT_NE(&cppgc::internal::Heap::From(heap2.get())->AsBase(),
+            HeapRegistry::TryFromManagedPointer(o));
+}
+
+}  // namespace internal
+}  // namespace cppgc

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

@@ -524,14 +524,41 @@ class LinkedNode final : public GarbageCollected<LinkedNode> {
 
 }  // namespace
 
-TEST_F(MemberHeapDeathTest, AssignDifferentHeapValues) {
-  auto* o1 = MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr);
-  auto* o2 = MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), o1);
+TEST_F(MemberHeapDeathTest, CheckForOffHeapMemberCrashesOnReassignment) {
+  std::vector<Member<LinkedNode>> off_heap_member;
+  // Verification state is constructed on first assignment.
+  off_heap_member.emplace_back(
+      MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr));
   {
     auto tmp_heap = cppgc::Heap::Create(platform_);
-    auto* o3 = MakeGarbageCollected<LinkedNode>(tmp_heap->GetAllocationHandle(),
-                                                nullptr);
-    EXPECT_DEATH_IF_SUPPORTED(o2->SetNext(o3), "");
+    auto* tmp_obj = MakeGarbageCollected<LinkedNode>(
+        tmp_heap->GetAllocationHandle(), nullptr);
+    EXPECT_DEATH_IF_SUPPORTED(off_heap_member[0] = tmp_obj, "");
+  }
+}
+
+TEST_F(MemberHeapDeathTest, CheckForOnStackMemberCrashesOnReassignment) {
+  Member<LinkedNode> stack_member;
+  // Verification state is constructed on first assignment.
+  stack_member =
+      MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr);
+  {
+    auto tmp_heap = cppgc::Heap::Create(platform_);
+    auto* tmp_obj = MakeGarbageCollected<LinkedNode>(
+        tmp_heap->GetAllocationHandle(), nullptr);
+    EXPECT_DEATH_IF_SUPPORTED(stack_member = tmp_obj, "");
+  }
+}
+
+TEST_F(MemberHeapDeathTest, CheckForOnHeapMemberCrashesOnInitialAssignment) {
+  auto* obj = MakeGarbageCollected<LinkedNode>(GetAllocationHandle(), nullptr);
+  {
+    auto tmp_heap = cppgc::Heap::Create(platform_);
+    EXPECT_DEATH_IF_SUPPORTED(
+        // For regular on-heap Member references the verification state is
+        // constructed eagerly on creating the reference.
+        MakeGarbageCollected<LinkedNode>(tmp_heap->GetAllocationHandle(), obj),
+        "");
   }
 }