global-handles: Fix ASAN fake stack handling

We previously assumed that a fake stack should be mapped back to a
real stack based on fake-stack offsets. This is not correct: Fake and
real stack are disjoint and both contain the corresponding slot
values.

For global handles this means that on-stack handles must be registered
using their real stack frame base to be able to purge them
occasionally based on the current stack address.

When dealing with a slot though, the GC can just dereference the slot
for a value, indeppendent of whether the slot is in a fake or real
frame.

Drive-by: Fix tests that do not want stack handles by creating
handles on heap.

Change-Id: I2c86c8e047bd0d48c24c2642b2b4dba284a93909
Bug: chromium:1139914
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2507720
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70897}

src/handles/global-handles.cc

@@ -5,6 +5,7 @@
 #include "src/handles/global-handles.h"
 
 #include <algorithm>
+#include <cstdint>
 #include <map>
 
 #include "src/api/api-inl.h"
@@ -749,13 +750,10 @@ class GlobalHandles::OnStackTracedNodeSpace final {
   void SetStackStart(void* stack_start) {
     CHECK(on_stack_nodes_.empty());
     stack_start_ =
-        GetStackAddressForSlot(reinterpret_cast<uintptr_t>(stack_start));
+        GetRealStackAddressForSlot(reinterpret_cast<uintptr_t>(stack_start));
   }
 
-  bool IsOnStack(uintptr_t slot) const {
-    const uintptr_t address = GetStackAddressForSlot(slot);
-    return stack_start_ >= address && address > GetCurrentStackPosition();
-  }
+  V8_INLINE bool IsOnStack(uintptr_t slot) const;
 
   void Iterate(RootVisitor* v);
   TracedNode* Acquire(Object value, uintptr_t address);
@@ -772,39 +770,69 @@ class GlobalHandles::OnStackTracedNodeSpace final {
     GlobalHandles* global_handles;
   };
 
-  uintptr_t GetStackAddressForSlot(uintptr_t slot) const;
+  // Returns the real stack frame if slot is part of a fake frame, and slot
+  // otherwise.
+  V8_INLINE uintptr_t GetRealStackAddressForSlot(uintptr_t slot) const;
 
-  // Keeps track of registered handles and their stack address. The data
-  // structure is cleaned on iteration and when adding new references using the
-  // current stack address.
+  // Keeps track of registered handles. The data structure is cleaned on
+  // iteration and when adding new references using the current stack address.
+  // Cleaning is based on current stack address and the key of the map which is
+  // slightly different for ASAN configs -- see below.
+#ifdef V8_USE_ADDRESS_SANITIZER
+  // Mapping from stack slots or real stack frames to the corresponding nodes.
+  // In case a reference is part of a fake frame, we map it to the real stack
+  // frame base instead of the actual stack slot. The list keeps all nodes for
+  // a particular real frame.
+  std::map<uintptr_t, std::list<NodeEntry>> on_stack_nodes_;
+#else   // !V8_USE_ADDRESS_SANITIZER
+  // Mapping from stack slots to the corresponding nodes. We don't expect
+  // aliasing with overlapping lifetimes of nodes.
   std::map<uintptr_t, NodeEntry> on_stack_nodes_;
+#endif  // !V8_USE_ADDRESS_SANITIZER
+
   uintptr_t stack_start_ = 0;
   GlobalHandles* global_handles_ = nullptr;
   size_t acquire_count_ = 0;
 };
 
-uintptr_t GlobalHandles::OnStackTracedNodeSpace::GetStackAddressForSlot(
+uintptr_t GlobalHandles::OnStackTracedNodeSpace::GetRealStackAddressForSlot(
     uintptr_t slot) const {
 #ifdef V8_USE_ADDRESS_SANITIZER
-  void* fake_stack = __asan_get_current_fake_stack();
-  if (fake_stack) {
-    void* fake_frame_start;
-    void* real_frame = __asan_addr_is_in_fake_stack(
-        fake_stack, reinterpret_cast<void*>(slot), &fake_frame_start, nullptr);
-    if (real_frame) {
-      return reinterpret_cast<uintptr_t>(real_frame) +
-             (slot - reinterpret_cast<uintptr_t>(fake_frame_start));
-    }
-  }
+  void* real_frame = __asan_addr_is_in_fake_stack(
+      __asan_get_current_fake_stack(), reinterpret_cast<void*>(slot), nullptr,
+      nullptr);
+  return real_frame ? reinterpret_cast<uintptr_t>(real_frame) : slot;
 #endif  // V8_USE_ADDRESS_SANITIZER
   return slot;
 }
 
+bool GlobalHandles::OnStackTracedNodeSpace::IsOnStack(uintptr_t slot) const {
+#ifdef V8_USE_ADDRESS_SANITIZER
+  if (__asan_addr_is_in_fake_stack(__asan_get_current_fake_stack(),
+                                   reinterpret_cast<void*>(slot), nullptr,
+                                   nullptr)) {
+    return true;
+  }
+#endif  // V8_USE_ADDRESS_SANITIZER
+  return stack_start_ >= slot && slot > GetCurrentStackPosition();
+}
+
 void GlobalHandles::OnStackTracedNodeSpace::NotifyEmptyEmbedderStack() {
   on_stack_nodes_.clear();
 }
 
 void GlobalHandles::OnStackTracedNodeSpace::Iterate(RootVisitor* v) {
+#ifdef V8_USE_ADDRESS_SANITIZER
+  for (auto& pair : on_stack_nodes_) {
+    for (auto& node_entry : pair.second) {
+      TracedNode& node = node_entry.node;
+      if (node.IsRetainer()) {
+        v->VisitRootPointer(Root::kGlobalHandles, "on-stack TracedReference",
+                            node.location());
+      }
+    }
+  }
+#else   // !V8_USE_ADDRESS_SANITIZER
   // Handles have been cleaned from the GC entry point which is higher up the
   // stack.
   for (auto& pair : on_stack_nodes_) {
@@ -814,6 +842,7 @@ void GlobalHandles::OnStackTracedNodeSpace::Iterate(RootVisitor* v) {
                           node.location());
     }
   }
+#endif  // !V8_USE_ADDRESS_SANITIZER
 }
 
 GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
@@ -828,8 +857,13 @@ GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
   NodeEntry entry;
   entry.node.Free(nullptr);
   entry.global_handles = global_handles_;
-  auto pair =
-      on_stack_nodes_.insert({GetStackAddressForSlot(slot), std::move(entry)});
+#ifdef V8_USE_ADDRESS_SANITIZER
+  auto pair = on_stack_nodes_.insert({GetRealStackAddressForSlot(slot), {}});
+  pair.first->second.push_back(std::move(entry));
+  TracedNode* result = &(pair.first->second.back().node);
+#else   // !V8_USE_ADDRESS_SANITIZER
+  auto pair = on_stack_nodes_.insert(
+      {GetRealStackAddressForSlot(slot), std::move(entry)});
   if (!pair.second) {
     // Insertion failed because there already was an entry present for that
     // stack address. This can happen because cleanup is conservative in which
@@ -838,6 +872,7 @@ GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
     pair.first->second.node.Free(nullptr);
   }
   TracedNode* result = &(pair.first->second.node);
+#endif  // !V8_USE_ADDRESS_SANITIZER
   result->Acquire(value);
   result->set_is_on_stack(true);
   return result;

test/cctest/heap/test-embedder-tracing.cc

@@ -324,13 +324,13 @@ void TracedGlobalTest(v8::Isolate* isolate,
   v8::Local<v8::Context> context = v8::Context::New(isolate);
   v8::Context::Scope context_scope(context);
 
-  v8::TracedGlobal<v8::Object> global;
-  construct_function(isolate, context, &global);
-  CHECK(InCorrectGeneration(isolate, global));
-  modifier_function(global);
+  auto global = std::make_unique<v8::TracedGlobal<v8::Object>>();
+  construct_function(isolate, context, global.get());
+  CHECK(InCorrectGeneration(isolate, *global));
+  modifier_function(*global);
   gc_function();
-  CHECK_IMPLIES(survives == SurvivalMode::kSurvives, !global.IsEmpty());
-  CHECK_IMPLIES(survives == SurvivalMode::kDies, global.IsEmpty());
+  CHECK_IMPLIES(survives == SurvivalMode::kSurvives, !global->IsEmpty());
+  CHECK_IMPLIES(survives == SurvivalMode::kDies, global->IsEmpty());
 }
 
 }  // namespace
@@ -371,33 +371,33 @@ TEST(TracedGlobalCopyWithDestructor) {
   i::GlobalHandles* global_handles = CcTest::i_isolate()->global_handles();
 
   const size_t initial_count = global_handles->handles_count();
-  v8::TracedGlobal<v8::Object> global1;
+  auto global1 = std::make_unique<v8::TracedGlobal<v8::Object>>();
   {
     v8::HandleScope scope(isolate);
-    global1.Reset(isolate, v8::Object::New(isolate));
+    global1->Reset(isolate, v8::Object::New(isolate));
   }
-  v8::TracedGlobal<v8::Object> global2(global1);
-  v8::TracedGlobal<v8::Object> global3;
-  global3 = global2;
+  auto global2 = std::make_unique<v8::TracedGlobal<v8::Object>>(*global1);
+  auto global3 = std::make_unique<v8::TracedGlobal<v8::Object>>();
+  *global3 = *global2;
   CHECK_EQ(initial_count + 3, global_handles->handles_count());
-  CHECK(!global1.IsEmpty());
-  CHECK_EQ(global1, global2);
-  CHECK_EQ(global2, global3);
+  CHECK(!global1->IsEmpty());
+  CHECK_EQ(*global1, *global2);
+  CHECK_EQ(*global2, *global3);
   {
     v8::HandleScope scope(isolate);
-    auto tmp = v8::Local<v8::Object>::New(isolate, global3);
+    auto tmp = v8::Local<v8::Object>::New(isolate, *global3);
     CHECK(!tmp.IsEmpty());
     InvokeMarkSweep();
   }
   CHECK_EQ(initial_count + 3, global_handles->handles_count());
-  CHECK(!global1.IsEmpty());
-  CHECK_EQ(global1, global2);
-  CHECK_EQ(global2, global3);
+  CHECK(!global1->IsEmpty());
+  CHECK_EQ(*global1, *global2);
+  CHECK_EQ(*global2, *global3);
   InvokeMarkSweep();
   CHECK_EQ(initial_count, global_handles->handles_count());
-  CHECK(global1.IsEmpty());
-  CHECK_EQ(global1, global2);
-  CHECK_EQ(global2, global3);
+  CHECK(global1->IsEmpty());
+  CHECK_EQ(*global1, *global2);
+  CHECK_EQ(*global2, *global3);
 }
 
 TEST(TracedGlobalCopyNoDestructor) {
@@ -408,28 +408,28 @@ TEST(TracedGlobalCopyNoDestructor) {
   i::GlobalHandles* global_handles = CcTest::i_isolate()->global_handles();
 
   const size_t initial_count = global_handles->handles_count();
-  v8::TracedReference<v8::Value> global1;
+  auto global1 = std::make_unique<v8::TracedReference<v8::Value>>();
   {
     v8::HandleScope scope(isolate);
-    global1.Reset(isolate, v8::Object::New(isolate));
+    global1->Reset(isolate, v8::Object::New(isolate));
   }
-  v8::TracedReference<v8::Value> global2(global1);
-  v8::TracedReference<v8::Value> global3;
-  global3 = global2;
+  auto global2 = std::make_unique<v8::TracedReference<v8::Value>>(*global1);
+  auto global3 = std::make_unique<v8::TracedReference<v8::Value>>();
+  *global3 = *global2;
   CHECK_EQ(initial_count + 3, global_handles->handles_count());
-  CHECK(!global1.IsEmpty());
-  CHECK_EQ(global1, global2);
-  CHECK_EQ(global2, global3);
+  CHECK(!global1->IsEmpty());
+  CHECK_EQ(*global1, *global2);
+  CHECK_EQ(*global2, *global3);
   {
     v8::HandleScope scope(isolate);
-    auto tmp = v8::Local<v8::Value>::New(isolate, global3);
+    auto tmp = v8::Local<v8::Value>::New(isolate, *global3);
     CHECK(!tmp.IsEmpty());
     InvokeMarkSweep();
   }
   CHECK_EQ(initial_count + 3, global_handles->handles_count());
-  CHECK(!global1.IsEmpty());
-  CHECK_EQ(global1, global2);
-  CHECK_EQ(global2, global3);
+  CHECK(!global1->IsEmpty());
+  CHECK_EQ(*global1, *global2);
+  CHECK_EQ(*global2, *global3);
   InvokeMarkSweep();
   CHECK_EQ(initial_count, global_handles->handles_count());
 }
@@ -544,15 +544,15 @@ TEST(TracedReferenceHandlesMarking) {
   CcTest::InitializeVM();
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);
-  v8::TracedReference<v8::Value> live;
-  v8::TracedReference<v8::Value> dead;
-  live.Reset(isolate, v8::Undefined(isolate));
-  dead.Reset(isolate, v8::Undefined(isolate));
+  auto live = std::make_unique<v8::TracedReference<v8::Value>>();
+  auto dead = std::make_unique<v8::TracedReference<v8::Value>>();
+  live->Reset(isolate, v8::Undefined(isolate));
+  dead->Reset(isolate, v8::Undefined(isolate));
   i::GlobalHandles* global_handles = CcTest::i_isolate()->global_handles();
   {
     TestEmbedderHeapTracer tracer;
     heap::TemporaryEmbedderHeapTracerScope tracer_scope(isolate, &tracer);
-    tracer.AddReferenceForTracing(&live);
+    tracer.AddReferenceForTracing(live.get());
     const size_t initial_count = global_handles->handles_count();
     InvokeMarkSweep();
     const size_t final_count = global_handles->handles_count();
@@ -563,7 +563,7 @@ TEST(TracedReferenceHandlesMarking) {
   {
     TestEmbedderHeapTracer tracer;
     heap::TemporaryEmbedderHeapTracerScope tracer_scope(isolate, &tracer);
-    tracer.AddReferenceForTracing(&live);
+    tracer.AddReferenceForTracing(live.get());
     const size_t initial_count = global_handles->handles_count();
     InvokeMarkSweep();
     const size_t final_count = global_handles->handles_count();
@@ -579,8 +579,8 @@ TEST(TracedReferenceHandlesDoNotLeak) {
   CcTest::InitializeVM();
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);
-  v8::TracedReference<v8::Value> ref;
-  ref.Reset(isolate, v8::Undefined(isolate));
+  auto ref = std::make_unique<v8::TracedReference<v8::Value>>();
+  ref->Reset(isolate, v8::Undefined(isolate));
   i::GlobalHandles* global_handles = CcTest::i_isolate()->global_handles();
   const size_t initial_count = global_handles->handles_count();
   // We need two GCs because handles are black allocated.
@@ -635,10 +635,10 @@ TEST(TracedGlobalIteration) {
   TestEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(isolate, &tracer);
 
-  v8::TracedGlobal<v8::Object> traced;
-  ConstructJSObject(isolate, isolate->GetCurrentContext(), &traced);
-  CHECK(!traced.IsEmpty());
-  traced.SetWrapperClassId(57);
+  auto traced = std::make_unique<v8::TracedGlobal<v8::Object>>();
+  ConstructJSObject(isolate, isolate->GetCurrentContext(), traced.get());
+  CHECK(!traced->IsEmpty());
+  traced->SetWrapperClassId(57);
   TracedGlobalVisitor visitor;
   {
     v8::HandleScope scope(isolate);
@@ -669,18 +669,18 @@ TEST(TracedGlobalSetFinalizationCallbackScavenge) {
   tracer.ConsiderTracedGlobalAsRoot(false);
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(isolate, &tracer);
 
-  v8::TracedGlobal<v8::Object> traced;
-  ConstructJSApiObject(isolate, isolate->GetCurrentContext(), &traced);
-  CHECK(!traced.IsEmpty());
+  auto traced = std::make_unique<v8::TracedGlobal<v8::Object>>();
+  ConstructJSApiObject(isolate, isolate->GetCurrentContext(), traced.get());
+  CHECK(!traced->IsEmpty());
   {
     v8::HandleScope scope(isolate);
-    auto local = traced.Get(isolate);
+    auto local = traced->Get(isolate);
     local->SetAlignedPointerInInternalField(0, reinterpret_cast<void*>(0x4));
     local->SetAlignedPointerInInternalField(1, reinterpret_cast<void*>(0x8));
   }
-  traced.SetFinalizationCallback(&traced, FinalizationCallback);
+  traced->SetFinalizationCallback(traced.get(), FinalizationCallback);
   heap::InvokeScavenge();
-  CHECK(traced.IsEmpty());
+  CHECK(traced->IsEmpty());
 }
 
 TEST(TracedGlobalSetFinalizationCallbackMarkSweep) {
@@ -691,18 +691,18 @@ TEST(TracedGlobalSetFinalizationCallbackMarkSweep) {
   TestEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(isolate, &tracer);
 
-  v8::TracedGlobal<v8::Object> traced;
-  ConstructJSApiObject(isolate, isolate->GetCurrentContext(), &traced);
-  CHECK(!traced.IsEmpty());
+  auto traced = std::make_unique<v8::TracedGlobal<v8::Object>>();
+  ConstructJSApiObject(isolate, isolate->GetCurrentContext(), traced.get());
+  CHECK(!traced->IsEmpty());
   {
     v8::HandleScope scope(isolate);
-    auto local = traced.Get(isolate);
+    auto local = traced->Get(isolate);
     local->SetAlignedPointerInInternalField(0, reinterpret_cast<void*>(0x4));
     local->SetAlignedPointerInInternalField(1, reinterpret_cast<void*>(0x8));
   }
-  traced.SetFinalizationCallback(&traced, FinalizationCallback);
+  traced->SetFinalizationCallback(traced.get(), FinalizationCallback);
   heap::InvokeMarkSweep();
-  CHECK(traced.IsEmpty());
+  CHECK(traced->IsEmpty());
 }
 
 TEST(TracePrologueCallingIntoV8WriteBarrier) {

test/cctest/test-global-handles.cc

@@ -158,13 +158,13 @@ void TracedGlobalTest(v8::Isolate* isolate,
   NonRootingEmbedderHeapTracer tracer;
   heap::TemporaryEmbedderHeapTracerScope tracer_scope(isolate, &tracer);
 
-  TracedGlobalWrapper fp;
-  construct_function(isolate, context, &fp);
-  CHECK(heap::InCorrectGeneration(isolate, fp.handle));
-  modifier_function(&fp);
+  auto fp = std::make_unique<TracedGlobalWrapper>();
+  construct_function(isolate, context, fp.get());
+  CHECK(heap::InCorrectGeneration(isolate, fp->handle));
+  modifier_function(fp.get());
   gc_function();
-  CHECK_IMPLIES(survives == SurvivalMode::kSurvives, !fp.handle.IsEmpty());
-  CHECK_IMPLIES(survives == SurvivalMode::kDies, fp.handle.IsEmpty());
+  CHECK_IMPLIES(survives == SurvivalMode::kSurvives, !fp->handle.IsEmpty());
+  CHECK_IMPLIES(survives == SurvivalMode::kDies, fp->handle.IsEmpty());
 }
 
 void ResurrectingFinalizer(