From 542a78458fb4570a5c87a10d6db53c5450bf2b7a Mon Sep 17 00:00:00 2001 From: Michael Lippautz Date: Wed, 23 Mar 2022 20:33:48 +0100 Subject: [PATCH] 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/+/3536651 Reviewed-by: Leszek Swirski Commit-Queue: Michael Lippautz Cr-Commit-Position: refs/heads/main@{#79587} --- test/cctest/cctest.cc | 137 ++++++++++++++++++ test/cctest/cctest.h | 78 ++++------ test/cctest/heap/test-incremental-marking.cc | 12 +- test/cctest/heap/test-memory-measurement.cc | 6 +- test/cctest/heap/test-unmapper.cc | 17 +-- test/cctest/test-allocation.cc | 5 +- test/cctest/test-api.cc | 10 +- test/cctest/test-profile-generator.cc | 14 +- test/cctest/test-trace-event.cc | 7 +- .../cctest/wasm/test-streaming-compilation.cc | 4 +- test/cctest/wasm/test-wasm-metrics.cc | 4 +- 11 files changed, 186 insertions(+), 108 deletions(-) diff --git a/test/cctest/cctest.cc b/test/cctest/cctest.cc index 99bca0a29b..e8b740b4e8 100644 --- a/test/cctest/cctest.cc +++ b/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 TestPlatform::GetForegroundTaskRunner( + v8::Isolate* isolate) { + return old_platform()->GetForegroundTaskRunner(isolate); +} + +void TestPlatform::CallOnWorkerThread(std::unique_ptr task) { + old_platform()->CallOnWorkerThread(std::move(task)); +} + +void TestPlatform::CallDelayedOnWorkerThread(std::unique_ptr task, + double delay_in_seconds) { + old_platform()->CallDelayedOnWorkerThread(std::move(task), delay_in_seconds); +} + +std::unique_ptr TestPlatform::PostJob( + v8::TaskPriority priority, std::unique_ptr 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(&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_); } diff --git a/test/cctest/cctest.h b/test/cctest/cctest.h index 735fc9cc9a..7297aeef4e 100644 --- a/test/cctest/cctest.h +++ b/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 GetForegroundTaskRunner( - v8::Isolate* isolate) override { - return old_platform()->GetForegroundTaskRunner(isolate); - } - - void CallOnWorkerThread(std::unique_ptr task) override { - old_platform()->CallOnWorkerThread(std::move(task)); - } - + v8::Isolate* isolate) override; + void CallOnWorkerThread(std::unique_ptr task) override; void CallDelayedOnWorkerThread(std::unique_ptr task, - double delay_in_seconds) override { - old_platform()->CallDelayedOnWorkerThread(std::move(task), - delay_in_seconds); - } - + double delay_in_seconds) override; std::unique_ptr PostJob( v8::TaskPriority priority, - std::unique_ptr 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 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 old_platform_; + bool active_ = false; }; #if defined(USE_SIMULATOR) diff --git a/test/cctest/heap/test-incremental-marking.cc b/test/cctest/heap/test-incremental-marking.cc index 702d66560e..cfc15ebafd 100644 --- a/test/cctest/heap/test-incremental-marking.cc +++ b/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 taskrunner_; std::vector> worker_tasks_; - v8::Platform* old_platform_; }; UNINITIALIZED_TEST(IncrementalMarkingUsingTasks) { diff --git a/test/cctest/heap/test-memory-measurement.cc b/test/cctest/heap/test-memory-measurement.cc index a5a0e6b645..d7b4dc5522 100644 --- a/test/cctest/heap/test-memory-measurement.cc +++ b/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 GetForegroundTaskRunner( v8::Isolate*) override { diff --git a/test/cctest/heap/test-unmapper.cc b/test/cctest/heap/test-unmapper.cc index 164de7571c..02694f3e69 100644 --- a/test/cctest/heap/test-unmapper.cc +++ b/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> worker_tasks_; - v8::Platform* old_platform_; }; UNINITIALIZED_TEST(EagerUnmappingInCollectAllAvailableGarbage) { diff --git a/test/cctest/test-allocation.cc b/test/cctest/test-allocation.cc index 2078aeb02a..e868a70c3b 100644 --- a/test/cctest/test-allocation.cc +++ b/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; } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index dd4b46fa48..99396c1af8 100644 --- a/test/cctest/test-api.cc +++ b/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; }; diff --git a/test/cctest/test-profile-generator.cc b/test/cctest/test-profile-generator.cc index 4564afd480..216cd280db 100644 --- a/test/cctest/test-profile-generator.cc +++ b/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 GetForegroundTaskRunner( v8::Isolate*) override { @@ -575,7 +570,6 @@ class MockPlatform : public TestPlatform { std::unique_ptr task_; }; - v8::Platform* old_platform_; std::shared_ptr mock_task_runner_; }; } // namespace diff --git a/test/cctest/test-trace-event.cc b/test/cctest/test-trace-event.cc index 2e2dd3d2cb..cd345565a2 100644 --- a/test/cctest/test-trace-event.cc +++ b/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_; diff --git a/test/cctest/wasm/test-streaming-compilation.cc b/test/cctest/wasm/test-streaming-compilation.cc index a6fc58f5d1..9e29059665 100644 --- a/test/cctest/wasm/test-streaming-compilation.cc +++ b/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()) { - // 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(); } diff --git a/test/cctest/wasm/test-wasm-metrics.cc b/test/cctest/wasm/test-wasm-metrics.cc index abb3dc9520..e575136764 100644 --- a/test/cctest/wasm/test-wasm-metrics.cc +++ b/test/cctest/wasm/test-wasm-metrics.cc @@ -25,11 +25,11 @@ namespace { class MockPlatform final : public TestPlatform { public: MockPlatform() : task_runner_(std::make_shared()) { - // 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(); }