platform, cppgc: Fix stack handling routines

mlippautz · Commit Bot · commit cfba2e5d4ee8 · 2020-10-30T15:07:26.000Z
- Provide GetRealStackAddressForSlot that deals with ASAN fake stacks properly, also accounting for the fact that ASAN gets its real stack address in a nested call. - Fix cppgc on-stack getter. - Reuse platform routines in global handles. Bug: chromium:1139914, chromium:1056170 Change-Id: If11a40d543b33edcea220bb70f170ac018e15053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2509594 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#70899}
diff --git a/src/base/platform/platform-aix.cc b/src/base/platform/platform-aix.cc
@@ -130,7 +130,7 @@ void OS::SignalCodeMovingGC() {}
 void OS::AdjustSchedulingParams() {}
 
 // static
-void* Stack::GetStackStart() {
+Stack::StackSlot Stack::GetStackStart() {
   // pthread_getthrds_np creates 3 values:
   // __pi_stackaddr, __pi_stacksize, __pi_stackend
 
diff --git a/src/base/platform/platform-freebsd.cc b/src/base/platform/platform-freebsd.cc
@@ -98,7 +98,7 @@ void OS::SignalCodeMovingGC() {}
 void OS::AdjustSchedulingParams() {}
 
 // static
-void* Stack::GetStackStart() {
+Stack::StackSlot Stack::GetStackStart() {
   pthread_attr_t attr;
   int error;
   pthread_attr_init(&attr);
diff --git a/src/base/platform/platform-macos.cc b/src/base/platform/platform-macos.cc
@@ -94,7 +94,7 @@ void OS::AdjustSchedulingParams() {
 }
 
 // static
-void* Stack::GetStackStart() {
+Stack::StackSlot Stack::GetStackStart() {
   return pthread_get_stackaddr_np(pthread_self());
 }
 
diff --git a/src/base/platform/platform-posix.cc b/src/base/platform/platform-posix.cc
@@ -1003,7 +1003,7 @@ void Thread::SetThreadLocal(LocalStorageKey key, void* value) {
     !defined(V8_OS_SOLARIS)
 
 // static
-void* Stack::GetStackStart() {
+Stack::StackSlot Stack::GetStackStart() {
   pthread_attr_t attr;
   int error = pthread_getattr_np(pthread_self(), &attr);
   if (!error) {
@@ -1029,7 +1029,9 @@ void* Stack::GetStackStart() {
         // !defined(_AIX) && !defined(V8_OS_SOLARIS)
 
 // static
-void* Stack::GetCurrentStackPosition() { return __builtin_frame_address(0); }
+Stack::StackSlot Stack::GetCurrentStackPosition() {
+  return __builtin_frame_address(0);
+}
 
 #undef LOG_TAG
 #undef MAP_ANONYMOUS
diff --git a/src/base/platform/platform-win32.cc b/src/base/platform/platform-win32.cc
@@ -1395,7 +1395,7 @@ void Thread::SetThreadLocal(LocalStorageKey key, void* value) {
 void OS::AdjustSchedulingParams() {}
 
 // static
-void* Stack::GetStackStart() {
+Stack::StackSlot Stack::GetStackStart() {
 #if defined(V8_TARGET_ARCH_X64)
   return reinterpret_cast<void*>(
       reinterpret_cast<NT_TIB64*>(NtCurrentTeb())->StackBase);
@@ -1414,7 +1414,7 @@ void* Stack::GetStackStart() {
 }
 
 // static
-void* Stack::GetCurrentStackPosition() {
+Stack::StackSlot Stack::GetCurrentStackPosition() {
 #if V8_CC_MSVC
   return _AddressOfReturnAddress();
 #else
diff --git a/src/base/platform/platform.h b/src/base/platform/platform.h
@@ -22,6 +22,7 @@
 #define V8_BASE_PLATFORM_PLATFORM_H_
 
 #include <cstdarg>
+#include <cstdint>
 #include <string>
 #include <vector>
 
@@ -433,30 +434,43 @@ class V8_BASE_EXPORT Thread {
 // TODO(v8:10354): Make use of the stack utilities here in V8.
 class V8_BASE_EXPORT Stack {
  public:
+  // Convenience wrapper to use stack slots as unsigned values or void*
+  // pointers.
+  struct StackSlot {
+    // NOLINTNEXTLINE
+    StackSlot(void* value) : value(reinterpret_cast<uintptr_t>(value)) {}
+    StackSlot(uintptr_t value) : value(value) {}  // NOLINT
+
+    // NOLINTNEXTLINE
+    operator void*() const { return reinterpret_cast<void*>(value); }
+    operator uintptr_t() const { return value; }  // NOLINT
+
+    uintptr_t value;
+  };
+
   // Gets the start of the stack of the current thread.
-  static void* GetStackStart();
+  static StackSlot GetStackStart();
 
   // Returns the current stack top. Works correctly with ASAN and SafeStack.
   // GetCurrentStackPosition() should not be inlined, because it works on stack
   // frames if it were inlined into a function with a huge stack frame it would
   // return an address significantly above the actual current stack position.
-  static V8_NOINLINE void* GetCurrentStackPosition();
+  static V8_NOINLINE StackSlot GetCurrentStackPosition();
 
-  // Translates an ASAN-based slot to a real stack slot if necessary.
-  static void* GetStackSlot(void* slot) {
+  // Returns the real stack frame if slot is part of a fake frame, and slot
+  // otherwise.
+  static StackSlot GetRealStackAddressForSlot(StackSlot slot) {
 #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, slot, &fake_frame_start, nullptr);
-      if (real_frame) {
-        return reinterpret_cast<void*>(
-            reinterpret_cast<uintptr_t>(real_frame) +
-            (reinterpret_cast<uintptr_t>(slot) -
-             reinterpret_cast<uintptr_t>(fake_frame_start)));
-      }
-    }
+    // ASAN fetches the real stack deeper in the __asan_addr_is_in_fake_stack()
+    // call (precisely, deeper in __asan_stack_malloc_()), which results in a
+    // real frame that could be outside of stack bounds. Adjust for this
+    // impreciseness here.
+    constexpr size_t kAsanRealFrameOffsetBytes = 32;
+    void* real_frame = __asan_addr_is_in_fake_stack(
+        __asan_get_current_fake_stack(), slot, nullptr, nullptr);
+    return real_frame
+               ? (static_cast<char*>(real_frame) + kAsanRealFrameOffsetBytes)
+               : slot;
 #endif  // V8_USE_ADDRESS_SANITIZER
     return slot;
   }
diff --git a/src/handles/global-handles.cc b/src/handles/global-handles.cc
@@ -749,8 +749,7 @@ class GlobalHandles::OnStackTracedNodeSpace final {
 
   void SetStackStart(void* stack_start) {
     CHECK(on_stack_nodes_.empty());
-    stack_start_ =
-        GetRealStackAddressForSlot(reinterpret_cast<uintptr_t>(stack_start));
+    stack_start_ = base::Stack::GetRealStackAddressForSlot(stack_start);
   }
 
   V8_INLINE bool IsOnStack(uintptr_t slot) const;
@@ -770,10 +769,6 @@ class GlobalHandles::OnStackTracedNodeSpace final {
     GlobalHandles* global_handles;
   };
 
-  // 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. 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
@@ -795,17 +790,6 @@ class GlobalHandles::OnStackTracedNodeSpace final {
   size_t acquire_count_ = 0;
 };
 
-uintptr_t GlobalHandles::OnStackTracedNodeSpace::GetRealStackAddressForSlot(
-    uintptr_t slot) const {
-#ifdef V8_USE_ADDRESS_SANITIZER
-  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(),
@@ -814,7 +798,7 @@ bool GlobalHandles::OnStackTracedNodeSpace::IsOnStack(uintptr_t slot) const {
     return true;
   }
 #endif  // V8_USE_ADDRESS_SANITIZER
-  return stack_start_ >= slot && slot > GetCurrentStackPosition();
+  return stack_start_ >= slot && slot > base::Stack::GetCurrentStackPosition();
 }
 
 void GlobalHandles::OnStackTracedNodeSpace::NotifyEmptyEmbedderStack() {
@@ -858,12 +842,13 @@ GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
   entry.node.Free(nullptr);
   entry.global_handles = global_handles_;
 #ifdef V8_USE_ADDRESS_SANITIZER
-  auto pair = on_stack_nodes_.insert({GetRealStackAddressForSlot(slot), {}});
+  auto pair = on_stack_nodes_.insert(
+      {base::Stack::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)});
+      {base::Stack::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
@@ -880,7 +865,8 @@ GlobalHandles::TracedNode* GlobalHandles::OnStackTracedNodeSpace::Acquire(
 
 void GlobalHandles::OnStackTracedNodeSpace::CleanupBelowCurrentStackPosition() {
   if (on_stack_nodes_.empty()) return;
-  const auto it = on_stack_nodes_.upper_bound(GetCurrentStackPosition());
+  const auto it =
+      on_stack_nodes_.upper_bound(base::Stack::GetCurrentStackPosition());
   on_stack_nodes_.erase(on_stack_nodes_.begin(), it);
 }
 
diff --git a/src/heap/base/stack.cc b/src/heap/base/stack.cc
@@ -20,9 +20,19 @@ extern "C" void PushAllRegistersAndIterateStack(const Stack*, StackVisitor*,
 Stack::Stack(const void* stack_start) : stack_start_(stack_start) {}
 
 bool Stack::IsOnStack(void* slot) const {
-  void* raw_slot = v8::base::Stack::GetStackSlot(slot);
-  return v8::base::Stack::GetCurrentStackPosition() <= raw_slot &&
-         raw_slot <= stack_start_;
+#ifdef V8_USE_ADDRESS_SANITIZER
+  // If the slot is part of a fake frame, then it is definitely on the stack.
+  void* real_frame = __asan_addr_is_in_fake_stack(
+      __asan_get_current_fake_stack(), reinterpret_cast<void*>(slot), nullptr,
+      nullptr);
+  if (real_frame) {
+    return true;
+  }
+  // Fall through as there is still a regular stack present even when running
+  // with ASAN fake stacks.
+#endif  // V8_USE_ADDRESS_SANITIZER
+  return v8::base::Stack::GetCurrentStackPosition() <= slot &&
+         slot <= stack_start_;
 }
 
 namespace {
diff --git a/test/unittests/base/platform/platform-unittest.cc b/test/unittests/base/platform/platform-unittest.cc
@@ -91,9 +91,12 @@ TEST(StackTest, GetCurrentStackPosition) {
 
 TEST(StackTest, StackVariableInBounds) {
   void* dummy;
-  ASSERT_GT(Stack::GetStackStart(), Stack::GetCurrentStackPosition());
-  EXPECT_GT(Stack::GetStackStart(), Stack::GetStackSlot(&dummy));
-  EXPECT_LT(Stack::GetCurrentStackPosition(), Stack::GetStackSlot(&dummy));
+  ASSERT_GT(static_cast<void*>(Stack::GetStackStart()),
+            Stack::GetCurrentStackPosition());
+  EXPECT_GT(static_cast<void*>(Stack::GetStackStart()),
+            Stack::GetRealStackAddressForSlot(&dummy));
+  EXPECT_LT(static_cast<void*>(Stack::GetCurrentStackPosition()),
+            Stack::GetRealStackAddressForSlot(&dummy));
 }
 
 }  // namespace base

Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,7 @@ void OS::AdjustSchedulingParams() {`
`94`	`94`	`}`
`95`	`95`
`96`	`96`	`// static`
`97`		`-void* Stack::GetStackStart() {`
	`97`	`+Stack::StackSlot Stack::GetStackStart() {`
`98`	`98`	`return pthread_get_stackaddr_np(pthread_self());`
`99`	`99`	`}`
`100`	`100`