[heap][wasm] Scan wasm inactive stacks

thibaudmichaud · V8 LUCI CQ · commit d9aa68e85079 · 2022-12-08T14:24:37.000Z
Wasm stack switching breaks the expectations of the unified V8/C++ heap by breaking the stack into multiple segments. To fix this: - Store a list of interesting inactive stacks in the heap's Stack object - When wasm switches stack, update this list, and also update the stack start pointer - Change {Stack::IteratePointers} to also visit pointers in the current list of inactive stacks R=nikolaos@chromium.org,jkummerow@chromium.org CC=​​irezvov@chromium.org Bug: v8:13493 Change-Id: Ieafeb89da31325e542e67403b6dc66c28d3be2fe Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4081126 Commit-Queue: Thibaud Michaud <thibaudm@chromium.org> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Nikolaos Papaspyrou <nikolaos@chromium.org> Cr-Commit-Position: refs/heads/main@{#84731}
diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc
@@ -3054,6 +3054,30 @@ void Isolate::AddSharedWasmMemory(Handle<WasmMemoryObject> memory_object) {
       this, shared_wasm_memories, MaybeObjectHandle::Weak(memory_object));
   heap()->set_shared_wasm_memories(*shared_wasm_memories);
 }
+
+void Isolate::RecordStackSwitchForScanning() {
+  Object current = root(RootIndex::kActiveContinuation);
+  DCHECK(!current.IsUndefined());
+  thread_local_top()->stack_.ClearStackSegments();
+  wasm::StackMemory* stack = Managed<wasm::StackMemory>::cast(
+                                 WasmContinuationObject::cast(current).stack())
+                                 .get()
+                                 .get();
+  current = WasmContinuationObject::cast(current).parent();
+  thread_local_top()->stack_.SetStackStart(
+      reinterpret_cast<void*>(stack->base()));
+  // We don't need to add all inactive stacks. Only the ones in the active chain
+  // may contain cpp heap pointers.
+  while (!current.IsUndefined()) {
+    auto cont = WasmContinuationObject::cast(current);
+    auto* stack = Managed<wasm::StackMemory>::cast(cont.stack()).get().get();
+    thread_local_top()->stack_.AddStackSegment(
+        reinterpret_cast<const void*>(stack->base()),
+        reinterpret_cast<const void*>(stack->jmpbuf()->sp));
+    current = cont.parent();
+  }
+}
+
 #endif  // V8_ENABLE_WEBASSEMBLY
 
 Isolate::PerIsolateThreadData::~PerIsolateThreadData() {
diff --git a/src/execution/isolate.h b/src/execution/isolate.h
@@ -2023,6 +2023,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
 #ifdef V8_ENABLE_WEBASSEMBLY
   wasm::StackMemory*& wasm_stacks() { return wasm_stacks_; }
+  // Update the thread local's Stack object so that it is aware of the new stack
+  // start and the inactive stacks.
+  void RecordStackSwitchForScanning();
 #endif
 
   // Access to the global "locals block list cache". Caches outer-stack
diff --git a/src/execution/thread-local-top.h b/src/execution/thread-local-top.h
@@ -30,7 +30,7 @@ class ThreadLocalTop {
   // TODO(all): This is not particularly beautiful. We should probably
   // refactor this to really consist of just Addresses and 32-bit
   // integer fields.
-  static constexpr uint32_t kSizeInBytes = 27 * kSystemPointerSize;
+  static constexpr uint32_t kSizeInBytes = 30 * kSystemPointerSize;
 
   // Does early low-level initialization that does not depend on the
   // isolate being present.
diff --git a/src/heap/base/stack.cc b/src/heap/base/stack.cc
@@ -15,6 +15,7 @@ namespace heap::base {
 Stack::Stack(const void* stack_start) : stack_start_(stack_start) {}
 
 void Stack::SetStackStart(const void* stack_start) {
+  DCHECK(!context_);
   stack_start_ = stack_start;
 }
 
@@ -61,29 +62,33 @@ void IterateAsanFakeFrameIfNecessary(StackVisitor* visitor,
   // native frame. In case |addr| points to a fake frame of the current stack
   // iterate the fake frame. Frame layout see
   // https://github.com/google/sanitizers/wiki/AddressSanitizerUseAfterReturn
-  if (asan_fake_stack) {
-    void* fake_frame_begin;
-    void* fake_frame_end;
-    void* real_stack_frame = __asan_addr_is_in_fake_stack(
-        const_cast<void*>(asan_fake_stack), const_cast<void*>(address),
-        &fake_frame_begin, &fake_frame_end);
-    if (real_stack_frame) {
-      // |address| points to a fake frame. Check that the fake frame is part
-      // of this stack.
-      if (stack_start >= real_stack_frame && real_stack_frame >= stack_end) {
-        // Iterate the fake frame.
-        for (const void* const* current =
-                 reinterpret_cast<const void* const*>(fake_frame_begin);
-             current < fake_frame_end; ++current) {
-          const void* address = *current;
-          if (address == nullptr) continue;
-          visitor->VisitPointer(address);
-        }
+  if (!asan_fake_stack) return;
+  void* fake_frame_begin;
+  void* fake_frame_end;
+  void* real_stack_frame = __asan_addr_is_in_fake_stack(
+      const_cast<void*>(asan_fake_stack), const_cast<void*>(address),
+      &fake_frame_begin, &fake_frame_end);
+  if (real_stack_frame) {
+    // |address| points to a fake frame. Check that the fake frame is part
+    // of this stack.
+    if (stack_start >= real_stack_frame && real_stack_frame >= stack_end) {
+      // Iterate the fake frame.
+      for (const void* const* current =
+               reinterpret_cast<const void* const*>(fake_frame_begin);
+           current < fake_frame_end; ++current) {
+        const void* address = *current;
+        if (address == nullptr) continue;
+        visitor->VisitPointer(address);
       }
     }
   }
 }
-
+#else
+void IterateAsanFakeFrameIfNecessary(StackVisitor* visitor,
+                                     const void* asan_fake_stack,
+                                     const void* stack_start,
+                                     const void* stack_end,
+                                     const void* address) {}
 #endif  // V8_USE_ADDRESS_SANITIZER
 
 void IterateUnsafeStackIfNecessary(StackVisitor* visitor) {
@@ -110,8 +115,6 @@ void IterateUnsafeStackIfNecessary(StackVisitor* visitor) {
 #endif  // defined(__has_feature)
 }
 
-}  // namespace
-
 // This method should never be inlined to ensure that a possible redzone cannot
 // contain any data that needs to be scanned.
 V8_NOINLINE
@@ -121,13 +124,32 @@ DISABLE_ASAN
 // thread, e.g., for interrupt handling. Atomic reads are not enough as the
 // other thread may use a lock to synchronize the access.
 DISABLE_TSAN
+void IteratePointersInStack(StackVisitor* visitor, const void* top,
+                            const void* start, const void* asan_fake_stack) {
+  for (const void* const* current = reinterpret_cast<const void* const*>(top);
+       current < start; ++current) {
+    // MSAN: Instead of unpoisoning the whole stack, the slot's value is copied
+    // into a local which is unpoisoned.
+    const void* address = *current;
+    MSAN_MEMORY_IS_INITIALIZED(&address, sizeof(address));
+    if (address == nullptr) continue;
+    visitor->VisitPointer(address);
+    IterateAsanFakeFrameIfNecessary(visitor, asan_fake_stack, start, top,
+                                    address);
+  }
+}
+
+}  // namespace
+
 void Stack::IteratePointers(StackVisitor* visitor) const {
   DCHECK_NOT_NULL(stack_start_);
   DCHECK(context_);
   DCHECK_NOT_NULL(context_->stack_marker);
 
 #ifdef V8_USE_ADDRESS_SANITIZER
   const void* asan_fake_stack = __asan_get_current_fake_stack();
+#else
+  const void* asan_fake_stack = nullptr;
 #endif  // V8_USE_ADDRESS_SANITIZER
 
   // Iterate through the registers.
@@ -136,10 +158,8 @@ void Stack::IteratePointers(StackVisitor* visitor) const {
     MSAN_MEMORY_IS_INITIALIZED(&address, sizeof(address));
     if (address == nullptr) continue;
     visitor->VisitPointer(address);
-#ifdef V8_USE_ADDRESS_SANITIZER
     IterateAsanFakeFrameIfNecessary(visitor, asan_fake_stack, stack_start_,
                                     context_->stack_marker, address);
-#endif  // V8_USE_ADDRESS_SANITIZER
   }
 
   // Iterate through the stack.
@@ -148,19 +168,12 @@ void Stack::IteratePointers(StackVisitor* visitor) const {
   constexpr size_t kMinStackAlignment = sizeof(void*);
   CHECK_EQ(0u, reinterpret_cast<uintptr_t>(context_->stack_marker) &
                    (kMinStackAlignment - 1));
-  for (const void* const* current =
-           reinterpret_cast<const void* const*>(context_->stack_marker);
-       current < stack_start_; ++current) {
-    // MSAN: Instead of unpoisoning the whole stack, the slot's value is copied
-    // into a local which is unpoisoned.
-    const void* address = *current;
-    MSAN_MEMORY_IS_INITIALIZED(&address, sizeof(address));
-    if (address == nullptr) continue;
-    visitor->VisitPointer(address);
-#ifdef V8_USE_ADDRESS_SANITIZER
-    IterateAsanFakeFrameIfNecessary(visitor, asan_fake_stack, stack_start_,
-                                    context_->stack_marker, address);
-#endif  // V8_USE_ADDRESS_SANITIZER
+  IteratePointersInStack(
+      visitor, reinterpret_cast<const void* const*>(context_->stack_marker),
+      stack_start_, asan_fake_stack);
+
+  for (const auto& stack : inactive_stacks_) {
+    IteratePointersInStack(visitor, stack.top, stack.start, asan_fake_stack);
   }
 
   IterateUnsafeStackIfNecessary(visitor);
@@ -219,4 +232,11 @@ void Stack::ClearContext(bool check_invariant) {
   context_.reset();
 }
 
+void Stack::AddStackSegment(const void* start, const void* top) {
+  DCHECK_LE(top, start);
+  inactive_stacks_.push_back({start, top});
+}
+
+void Stack::ClearStackSegments() { inactive_stacks_.clear(); }
+
 }  // namespace heap::base
diff --git a/src/heap/base/stack.h b/src/heap/base/stack.h
@@ -23,6 +23,9 @@ class StackVisitor {
 // - ASAN/MSAN;
 // - SafeStack: https://releases.llvm.org/10.0.0/tools/clang/docs/SafeStack.html
 //
+// Stacks grow down, so throughout this class "start" refers the highest
+// address of the stack, and top/marker the lowest.
+//
 // TODO(chromium:1056170): Consider adding a component that keeps track
 // of relevant GC stack regions where interesting pointers can be found.
 class V8_EXPORT_PRIVATE Stack final {
@@ -44,7 +47,7 @@ class V8_EXPORT_PRIVATE Stack final {
 
   // Word-aligned iteration of the stack and the saved registers.
   // Slot values are passed on to `visitor`.
-  V8_NOINLINE void IteratePointers(StackVisitor* visitor) const;
+  void IteratePointers(StackVisitor* visitor) const;
 
   // Saves and clears the stack context, i.e., it sets the stack marker and
   // saves the registers.
@@ -54,6 +57,9 @@ class V8_EXPORT_PRIVATE Stack final {
   void SaveContext(bool check_invariant = true);
   void ClearContext(bool check_invariant = true);
 
+  void AddStackSegment(const void* start, const void* top);
+  void ClearStackSegments();
+
  private:
   struct Context {
     // The following constant is architecture-specific.
@@ -111,6 +117,14 @@ class V8_EXPORT_PRIVATE Stack final {
 
   const void* stack_start_;
   std::unique_ptr<Context> context_;
+
+  // Stack segments that may also contain pointers and should be
+  // scanned.
+  struct StackSegments {
+    const void* start;
+    const void* top;
+  };
+  std::vector<StackSegments> inactive_stacks_;
 };
 
 }  // namespace heap::base
diff --git a/src/runtime/runtime-wasm.cc b/src/runtime/runtime-wasm.cc
@@ -828,6 +828,7 @@ void SyncStackLimit(Isolate* isolate) {
   }
   uintptr_t limit = reinterpret_cast<uintptr_t>(stack->jmpbuf()->stack_limit);
   isolate->stack_guard()->SetStackLimit(limit);
+  isolate->RecordStackSwitchForScanning();
 }
 }  // namespace
 

Original file line number	Diff line number	Diff line change
`@@ -828,6 +828,7 @@ void SyncStackLimit(Isolate* isolate) {`
`828`	`828`	`}`
`829`	`829`	`uintptr_t limit = reinterpret_cast<uintptr_t>(stack->jmpbuf()->stack_limit);`
`830`	`830`	`isolate->stack_guard()->SetStackLimit(limit);`
	`831`	`+ isolate->RecordStackSwitchForScanning();`
`831`	`832`	`}`
`832`	`833`	`} // namespace`
`833`	`834`