Handles: Fix OnStackTracedNodeSpace with -fsanitize=safe-stack

When the stack is split in safe and unsafe parts, on-stack
TracedReferences are allocated on the unsafe stack. What currently
happens is that on GC we destroy all the on-stack references below the
current frame of the *safe* stack. If the safe stack is allocated above
the unsafe counterpart, then all the traced references will be
preliminary destructed on GC. This CL fixes it by using
__builtin___get_unsafe_stack_ptr() if -fsanitize=safe-stack is enabled.

In addition, deduplicate OnStackTracedNodeSpace::IsOnStack() and
Stack::IsOnStack() and move more logic into ::heap::base::Stack.

Bug: chromium:1278780
Change-Id: I9582bb1321958b7ec8ef2c0c46b9e42d51bb6f94
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3395033Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Auto-Submit: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78660}

src/handles/global-handles.cc

@@ -13,6 +13,7 @@
 #include "src/base/compiler-specific.h"
 #include "src/base/sanitizer/asan.h"
 #include "src/execution/vm-state-inl.h"
+#include "src/heap/base/stack.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/heap-inl.h"
 #include "src/heap/heap-write-barrier-inl.h"
@@ -753,7 +754,7 @@ class GlobalHandles::OnStackTracedNodeSpace final {
 
   void SetStackStart(void* stack_start) {
     CHECK(on_stack_nodes_.empty());
-    stack_start_ = base::Stack::GetRealStackAddressForSlot(stack_start);
+    stack_.SetStackStart(base::Stack::GetRealStackAddressForSlot(stack_start));
   }
 
   V8_INLINE bool IsOnStack(uintptr_t slot) const;
@@ -789,28 +790,17 @@ class GlobalHandles::OnStackTracedNodeSpace final {
   std::map<uintptr_t, NodeEntry> on_stack_nodes_;
 #endif  // !V8_USE_ADDRESS_SANITIZER
 
-  uintptr_t stack_start_ = 0;
+  ::heap::base::Stack stack_;
   GlobalHandles* global_handles_ = nullptr;
   size_t acquire_count_ = 0;
 };
 
 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
-#if defined(__has_feature)
-#if __has_feature(safe_stack)
-  if (reinterpret_cast<uintptr_t>(__builtin___get_unsafe_stack_top()) >= slot &&
-      slot > reinterpret_cast<uintptr_t>(__builtin___get_unsafe_stack_ptr())) {
-    return true;
-  }
-#endif  // __has_feature(safe_stack)
-#endif  // defined(__has_feature)
-  return stack_start_ >= slot && slot > base::Stack::GetCurrentStackPosition();
+  // By the time this function is called, the stack start may not be set (i.e.
+  // SetStackStart() was not called). In that case, assume the slot is not on
+  // stack.
+  if (!stack_.stack_start()) return false;
+  return stack_.IsOnStack(reinterpret_cast<void*>(slot));
 }
 
 void GlobalHandles::OnStackTracedNodeSpace::NotifyEmptyEmbedderStack() {
@@ -877,8 +867,9 @@ GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
 
 void GlobalHandles::OnStackTracedNodeSpace::CleanupBelowCurrentStackPosition() {
   if (on_stack_nodes_.empty()) return;
-  const auto it =
-      on_stack_nodes_.upper_bound(base::Stack::GetCurrentStackPosition());
+  const uintptr_t stack_ptr = reinterpret_cast<uintptr_t>(
+      ::heap::base::Stack::GetCurrentStackPointerForLocalVariables());
+  const auto it = on_stack_nodes_.upper_bound(stack_ptr);
   on_stack_nodes_.erase(on_stack_nodes_.begin(), it);
 }
 

src/heap/base/stack.cc

@@ -21,7 +21,12 @@ extern "C" void PushAllRegistersAndIterateStack(const Stack*, StackVisitor*,
 
 Stack::Stack(const void* stack_start) : stack_start_(stack_start) {}
 
+void Stack::SetStackStart(const void* stack_start) {
+  stack_start_ = stack_start;
+}
+
 bool Stack::IsOnStack(void* slot) const {
+  DCHECK_NOT_NULL(stack_start_);
 #ifdef V8_USE_ADDRESS_SANITIZER
   // If the slot is part of a fake frame, then it is definitely on the stack.
   if (__asan_addr_is_in_fake_stack(__asan_get_current_fake_stack(),
@@ -86,7 +91,7 @@ void IterateAsanFakeFrameIfNecessary(StackVisitor* visitor,
 
 #endif  // V8_USE_ADDRESS_SANITIZER
 
-void IterateSafeStackIfNecessary(StackVisitor* visitor) {
+void IterateUnsafeStackIfNecessary(StackVisitor* visitor) {
 #if defined(__has_feature)
 #if __has_feature(safe_stack)
   // Source:
@@ -146,11 +151,12 @@ void IteratePointersImpl(const Stack* stack, StackVisitor* visitor,
 }  // namespace
 
 void Stack::IteratePointers(StackVisitor* visitor) const {
+  DCHECK_NOT_NULL(stack_start_);
   PushAllRegistersAndIterateStack(this, visitor, &IteratePointersImpl);
   // No need to deal with callee-saved registers as they will be kept alive by
   // the regular conservative stack iteration.
   // TODO(chromium:1056170): Add support for SIMD and/or filtering.
-  IterateSafeStackIfNecessary(visitor);
+  IterateUnsafeStackIfNecessary(visitor);
 }
 
 void Stack::IteratePointersUnsafe(StackVisitor* visitor,
@@ -158,5 +164,17 @@ void Stack::IteratePointersUnsafe(StackVisitor* visitor,
   IteratePointersImpl(this, visitor, reinterpret_cast<intptr_t*>(stack_end));
 }
 
+const void* Stack::GetCurrentStackPointerForLocalVariables() {
+#if defined(__has_feature)
+#if __has_feature(safe_stack)
+  return __builtin___get_unsafe_stack_ptr();
+#else   // __has_feature(safe_stack)
+  return v8::base::Stack::GetCurrentStackPosition();
+#endif  // __has_feature(safe_stack)
+#else   // defined(__has_feature)
+  return v8::base::Stack::GetCurrentStackPosition();
+#endif  // defined(__has_feature)
+}
+
 }  // namespace base
 }  // namespace heap

src/heap/base/stack.h

@@ -21,7 +21,10 @@ class StackVisitor {
 // - SafeStack: https://releases.llvm.org/10.0.0/tools/clang/docs/SafeStack.html
 class V8_EXPORT_PRIVATE Stack final {
  public:
-  explicit Stack(const void* stack_start);
+  explicit Stack(const void* stack_start = nullptr);
+
+  // Sets the start of the stack.
+  void SetStackStart(const void* stack_start);
 
   // Returns true if |slot| is part of the stack and false otherwise.
   bool IsOnStack(void* slot) const;
@@ -43,6 +46,12 @@ class V8_EXPORT_PRIVATE Stack final {
   // Returns the start of the stack.
   const void* stack_start() const { return stack_start_; }
 
+  // Get the current stack pointer for the stack, on which local variables are
+  // stored. In case the safe-stack is enabled (-fsanitize=safe-stack), this
+  // will return the stack pointer for the unsafe-stack. Otherwise, the function
+  // returns the stack pointer for the native stack.
+  static const void* GetCurrentStackPointerForLocalVariables();
+
  private:
   const void* stack_start_;
 };