MockTracingPlatform: Fix uaf with stack-scoped platform

This fixes a general race with stack-scoped `TestPlatform` which
may go out of scope while tasks on workers are still running.

Add a barrier for workers, implemented through tasks, to synchronize
destruction of `TestPlatform`.

While this fixes general races, such short-lived platforms still
break if tasks cache the global platform pointer.

Bug: v8:12635
Change-Id: Ifc6ecc29f0e2b7297ca52051eae9bd81013b60ce
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3536651Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79587}

test/cctest/cctest.cc

@@ -35,6 +35,9 @@
 #include "include/v8-isolate.h"
 #include "include/v8-local-handle.h"
 #include "include/v8-locker.h"
+#include "src/base/platform/condition-variable.h"
+#include "src/base/platform/mutex.h"
+#include "src/base/platform/semaphore.h"
 #include "src/base/strings.h"
 #include "src/codegen/compiler.h"
 #include "src/codegen/optimized-compilation-info.h"
@@ -458,3 +461,137 @@ ManualGCScope::~ManualGCScope() {
   i::FLAG_detect_ineffective_gcs_near_heap_limit =
       flag_detect_ineffective_gcs_near_heap_limit_;
 }
+
+TestPlatform::TestPlatform() : old_platform_(i::V8::GetCurrentPlatform()) {}
+
+void TestPlatform::NotifyPlatformReady() {
+  i::V8::SetPlatformForTesting(this);
+  CHECK(!active_);
+  active_ = true;
+}
+
+v8::PageAllocator* TestPlatform::GetPageAllocator() {
+  return old_platform()->GetPageAllocator();
+}
+
+void TestPlatform::OnCriticalMemoryPressure() {
+  old_platform()->OnCriticalMemoryPressure();
+}
+
+bool TestPlatform::OnCriticalMemoryPressure(size_t length) {
+  return old_platform()->OnCriticalMemoryPressure(length);
+}
+
+int TestPlatform::NumberOfWorkerThreads() {
+  return old_platform()->NumberOfWorkerThreads();
+}
+
+std::shared_ptr<v8::TaskRunner> TestPlatform::GetForegroundTaskRunner(
+    v8::Isolate* isolate) {
+  return old_platform()->GetForegroundTaskRunner(isolate);
+}
+
+void TestPlatform::CallOnWorkerThread(std::unique_ptr<v8::Task> task) {
+  old_platform()->CallOnWorkerThread(std::move(task));
+}
+
+void TestPlatform::CallDelayedOnWorkerThread(std::unique_ptr<v8::Task> task,
+                                             double delay_in_seconds) {
+  old_platform()->CallDelayedOnWorkerThread(std::move(task), delay_in_seconds);
+}
+
+std::unique_ptr<v8::JobHandle> TestPlatform::PostJob(
+    v8::TaskPriority priority, std::unique_ptr<v8::JobTask> job_task) {
+  return old_platform()->PostJob(priority, std::move(job_task));
+}
+
+double TestPlatform::MonotonicallyIncreasingTime() {
+  return old_platform()->MonotonicallyIncreasingTime();
+}
+
+double TestPlatform::CurrentClockTimeMillis() {
+  return old_platform()->CurrentClockTimeMillis();
+}
+
+bool TestPlatform::IdleTasksEnabled(v8::Isolate* isolate) {
+  return old_platform()->IdleTasksEnabled(isolate);
+}
+
+v8::TracingController* TestPlatform::GetTracingController() {
+  return old_platform()->GetTracingController();
+}
+
+namespace {
+
+class ShutdownTask final : public v8::Task {
+ public:
+  ShutdownTask(v8::base::Semaphore* destruction_barrier,
+               v8::base::Mutex* destruction_mutex,
+               v8::base::ConditionVariable* destruction_condition,
+               bool* can_destruct)
+      : destruction_barrier_(destruction_barrier),
+        destruction_mutex_(destruction_mutex),
+        destruction_condition_(destruction_condition),
+        can_destruct_(can_destruct)
+
+  {}
+
+  void Run() final {
+    destruction_barrier_->Signal();
+    {
+      v8::base::MutexGuard guard(destruction_mutex_);
+      while (!*can_destruct_) {
+        destruction_condition_->Wait(destruction_mutex_);
+      }
+    }
+    destruction_barrier_->Signal();
+  }
+
+ private:
+  v8::base::Semaphore* const destruction_barrier_;
+  v8::base::Mutex* const destruction_mutex_;
+  v8::base::ConditionVariable* const destruction_condition_;
+  bool* const can_destruct_;
+};
+
+}  // namespace
+
+void TestPlatform::RemovePlatform() {
+  DCHECK_EQ(i::V8::GetCurrentPlatform(), this);
+  // Destruction helpers.
+  // Barrier to wait until all shutdown tasks actually run (and subsequently
+  // block).
+  v8::base::Semaphore destruction_barrier{0};
+  // Primitives for blocking until `can_destruct` is true.
+  v8::base::Mutex destruction_mutex;
+  v8::base::ConditionVariable destruction_condition;
+  bool can_destruct = false;
+
+  for (int i = 0; i < NumberOfWorkerThreads(); i++) {
+    old_platform()->CallOnWorkerThread(
+        std::make_unique<ShutdownTask>(&destruction_barrier, &destruction_mutex,
+                                       &destruction_condition, &can_destruct));
+  }
+  // Wait till all worker threads reach the barrier.
+  for (int i = 0; i < NumberOfWorkerThreads(); i++) {
+    destruction_barrier.Wait();
+  }
+  // At this point all worker threads are blocked, so the platform can be
+  // swapped back.
+  i::V8::SetPlatformForTesting(old_platform_);
+  CHECK(active_);
+  active_ = false;
+  // Release all worker threads again.
+  {
+    v8::base::MutexGuard guard(&destruction_mutex);
+    can_destruct = true;
+    destruction_condition.NotifyAll();
+  }
+  // Wait till all worker threads resume. This is necessary as the threads would
+  // otherwise try to unlock `destruction_mutex` which may already be gone.
+  for (int i = 0; i < NumberOfWorkerThreads(); i++) {
+    destruction_barrier.Wait();
+  }
+}
+
+TestPlatform::~TestPlatform() { CHECK(!active_); }

test/cctest/cctest.h

@@ -700,76 +700,48 @@ class V8_NODISCARD ManualGCScope {
   const bool flag_detect_ineffective_gcs_near_heap_limit_;
 };
 
-// This is an abstract base class that can be overridden to implement a test
-// platform. It delegates all operations to a given platform at the time
-// of construction.
+// This is a base class that can be overridden to implement a test platform. It
+// delegates all operations to a given platform at the time of construction.
+// Breaks if tasks cache the platform themselves.
 class TestPlatform : public v8::Platform {
  public:
+  // Users inheriting from `TestPlatform` need to invoke `NotifyPlatformReady()`
+  // at the end of their constructor.
+  void NotifyPlatformReady();
+
+  // Eagerly removes the platform from being used by V8.
+  void RemovePlatform();
+
   TestPlatform(const TestPlatform&) = delete;
   TestPlatform& operator=(const TestPlatform&) = delete;
 
   // v8::Platform implementation.
-  v8::PageAllocator* GetPageAllocator() override {
-    return old_platform()->GetPageAllocator();
-  }
-
-  void OnCriticalMemoryPressure() override {
-    old_platform()->OnCriticalMemoryPressure();
-  }
-
-  bool OnCriticalMemoryPressure(size_t length) override {
-    return old_platform()->OnCriticalMemoryPressure(length);
-  }
-
-  int NumberOfWorkerThreads() override {
-    return old_platform()->NumberOfWorkerThreads();
-  }
-
+  v8::PageAllocator* GetPageAllocator() override;
+  void OnCriticalMemoryPressure() override;
+  bool OnCriticalMemoryPressure(size_t length) override;
+  int NumberOfWorkerThreads() override;
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
-      v8::Isolate* isolate) override {
-    return old_platform()->GetForegroundTaskRunner(isolate);
-  }
-
-  void CallOnWorkerThread(std::unique_ptr<v8::Task> task) override {
-    old_platform()->CallOnWorkerThread(std::move(task));
-  }
-
+      v8::Isolate* isolate) override;
+  void CallOnWorkerThread(std::unique_ptr<v8::Task> task) override;
   void CallDelayedOnWorkerThread(std::unique_ptr<v8::Task> task,
-                                 double delay_in_seconds) override {
-    old_platform()->CallDelayedOnWorkerThread(std::move(task),
-                                              delay_in_seconds);
-  }
-
+                                 double delay_in_seconds) override;
   std::unique_ptr<v8::JobHandle> PostJob(
       v8::TaskPriority priority,
-      std::unique_ptr<v8::JobTask> job_task) override {
-    return old_platform()->PostJob(priority, std::move(job_task));
-  }
-
-  double MonotonicallyIncreasingTime() override {
-    return old_platform()->MonotonicallyIncreasingTime();
-  }
-
-  double CurrentClockTimeMillis() override {
-    return old_platform()->CurrentClockTimeMillis();
-  }
-
-  bool IdleTasksEnabled(v8::Isolate* isolate) override {
-    return old_platform()->IdleTasksEnabled(isolate);
-  }
-
-  v8::TracingController* GetTracingController() override {
-    return old_platform()->GetTracingController();
-  }
+      std::unique_ptr<v8::JobTask> job_task) override;
+  double MonotonicallyIncreasingTime() override;
+  double CurrentClockTimeMillis() override;
+  bool IdleTasksEnabled(v8::Isolate* isolate) override;
+  v8::TracingController* GetTracingController() override;
 
  protected:
-  TestPlatform() : old_platform_(i::V8::GetCurrentPlatform()) {}
-  ~TestPlatform() override { i::V8::SetPlatformForTesting(old_platform_); }
+  TestPlatform();
+  ~TestPlatform() override;
 
   v8::Platform* old_platform() const { return old_platform_; }
 
  private:
   std::atomic<v8::Platform*> old_platform_;
+  bool active_ = false;
 };
 
 #if defined(USE_SIMULATOR)

test/cctest/heap/test-incremental-marking.cc

@@ -35,16 +35,11 @@ namespace heap {
 
 class MockPlatform : public TestPlatform {
  public:
-  MockPlatform()
-      : taskrunner_(new MockTaskRunner()),
-        old_platform_(i::V8::GetCurrentPlatform()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
-  }
+  MockPlatform() : taskrunner_(new MockTaskRunner()) { NotifyPlatformReady(); }
   ~MockPlatform() override {
-    i::V8::SetPlatformForTesting(old_platform_);
+    RemovePlatform();
     for (auto& task : worker_tasks_) {
-      old_platform_->CallOnWorkerThread(std::move(task));
+      old_platform()->CallOnWorkerThread(std::move(task));
     }
     worker_tasks_.clear();
   }
@@ -106,7 +101,6 @@ class MockPlatform : public TestPlatform {
 
   std::shared_ptr<MockTaskRunner> taskrunner_;
   std::vector<std::unique_ptr<Task>> worker_tasks_;
-  v8::Platform* old_platform_;
 };
 
 UNINITIALIZED_TEST(IncrementalMarkingUsingTasks) {

test/cctest/heap/test-memory-measurement.cc

@@ -132,10 +132,10 @@ namespace {
 
 class MockPlatform : public TestPlatform {
  public:
-  MockPlatform() : TestPlatform(), mock_task_runner_(new MockTaskRunner()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
+  MockPlatform() : mock_task_runner_(new MockTaskRunner()) {
+    NotifyPlatformReady();
   }
+  ~MockPlatform() override { RemovePlatform(); }
 
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
       v8::Isolate*) override {

test/cctest/heap/test-unmapper.cc

@@ -20,16 +20,11 @@ namespace heap {
 
 class MockPlatformForUnmapper : public TestPlatform {
  public:
-  MockPlatformForUnmapper()
-      : task_(nullptr), old_platform_(i::V8::GetCurrentPlatform()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
-  }
+  MockPlatformForUnmapper() { NotifyPlatformReady(); }
   ~MockPlatformForUnmapper() override {
-    delete task_;
-    i::V8::SetPlatformForTesting(old_platform_);
+    RemovePlatform();
     for (auto& task : worker_tasks_) {
-      old_platform_->CallOnWorkerThread(std::move(task));
+      old_platform()->CallOnWorkerThread(std::move(task));
     }
     worker_tasks_.clear();
   }
@@ -40,14 +35,8 @@ class MockPlatformForUnmapper : public TestPlatform {
 
   bool IdleTasksEnabled(v8::Isolate* isolate) override { return false; }
 
-  int NumberOfWorkerThreads() override {
-    return old_platform_->NumberOfWorkerThreads();
-  }
-
  private:
-  Task* task_;
   std::vector<std::unique_ptr<Task>> worker_tasks_;
-  v8::Platform* old_platform_;
 };
 
 UNINITIALIZED_TEST(EagerUnmappingInCollectAllAvailableGarbage) {

test/cctest/test-allocation.cc

@@ -34,10 +34,9 @@ class AllocationPlatform : public TestPlatform {
  public:
   AllocationPlatform() {
     current_platform = this;
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
+    NotifyPlatformReady();
   }
-  ~AllocationPlatform() override = default;
+  ~AllocationPlatform() override { RemovePlatform(); }
 
   void OnCriticalMemoryPressure() override { oom_callback_called = true; }
 

test/cctest/test-api.cc

@@ -23297,13 +23297,10 @@ TEST(ThrowOnJavascriptExecution) {
 
 namespace {
 
-class MockPlatform : public TestPlatform {
+class MockPlatform final : public TestPlatform {
  public:
-  MockPlatform() : old_platform_(i::V8::GetCurrentPlatform()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
-  }
-  ~MockPlatform() override { i::V8::SetPlatformForTesting(old_platform_); }
+  MockPlatform() { NotifyPlatformReady(); }
+  ~MockPlatform() final { RemovePlatform(); }
 
   bool dump_without_crashing_called() const {
     return dump_without_crashing_called_;
@@ -23312,7 +23309,6 @@ class MockPlatform : public TestPlatform {
   void DumpWithoutCrashing() override { dump_without_crashing_called_ = true; }
 
  private:
-  v8::Platform* old_platform_;
   bool dump_without_crashing_called_ = false;
 };
 

test/cctest/test-profile-generator.cc

@@ -528,17 +528,12 @@ class DiscardedSamplesDelegateImpl : public v8::DiscardedSamplesDelegate {
   void Notify() override {}
 };
 
-class MockPlatform : public TestPlatform {
+class MockPlatform final : public TestPlatform {
  public:
-  MockPlatform()
-      : old_platform_(i::V8::GetCurrentPlatform()),
-        mock_task_runner_(new MockTaskRunner()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
+  MockPlatform() : mock_task_runner_(new MockTaskRunner()) {
+    NotifyPlatformReady();
   }
-
-  // When done, explicitly revert to old_platform_.
-  ~MockPlatform() override { i::V8::SetPlatformForTesting(old_platform_); }
+  ~MockPlatform() override { RemovePlatform(); }
 
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(
       v8::Isolate*) override {
@@ -575,7 +570,6 @@ class MockPlatform : public TestPlatform {
     std::unique_ptr<Task> task_;
   };
 
-  v8::Platform* old_platform_;
   std::shared_ptr<MockTaskRunner> mock_task_runner_;
 };
 }  // namespace

test/cctest/test-trace-event.cc

@@ -86,11 +86,8 @@ class MockTracingController : public v8::TracingController {
 
 class MockTracingPlatform : public TestPlatform {
  public:
-  MockTracingPlatform() {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
-  }
-  ~MockTracingPlatform() override = default;
+  MockTracingPlatform() { NotifyPlatformReady(); }
+  ~MockTracingPlatform() override { RemovePlatform(); }
 
   v8::TracingController* GetTracingController() override {
     return &tracing_controller_;

test/cctest/wasm/test-streaming-compilation.cc

@@ -30,11 +30,11 @@ namespace wasm {
 class MockPlatform final : public TestPlatform {
  public:
   MockPlatform() : task_runner_(std::make_shared<MockTaskRunner>()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
+    NotifyPlatformReady();
   }
 
   ~MockPlatform() {
+    RemovePlatform();
     for (auto* job_handle : job_handles_) job_handle->ResetPlatform();
   }
 

test/cctest/wasm/test-wasm-metrics.cc

@@ -25,11 +25,11 @@ namespace {
 class MockPlatform final : public TestPlatform {
  public:
   MockPlatform() : task_runner_(std::make_shared<MockTaskRunner>()) {
-    // Now that it's completely constructed, make this the current platform.
-    i::V8::SetPlatformForTesting(this);
+    NotifyPlatformReady();
   }
 
   ~MockPlatform() override {
+    RemovePlatform();
     for (auto* job_handle : job_handles_) job_handle->ResetPlatform();
   }