From e864dec2e98bb0786c015aabc712cf978b5e3634 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Wed, 7 Aug 2024 14:11:18 +0000 Subject: [PATCH] VSyncEncodeAdapterMode: avoid UAF. This CL fixes a problem where VSEAM's `queue_` was dereferenced post destruction. A sequence triggering it is: 0. FrameCadenceAdapter (FCA) is configured with Metronome with >= 34 ms tick delay. 1. A frame is queued for processing on worker queue. 2. The FCA is destroyed. The contained VSyncEncodeAdapterMode instance is scheduled for deletion on worker queue. 3. Encode queue is destroyed. 4. Worker queue is executed, which runs a task that dereferences `queue_`. Bug: chromium:356423094 Change-Id: Iae8dc070304ef5ec0cfb0b4f27bbb7fe86e7def7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/358660 Commit-Queue: Markus Handell Reviewed-by: Danil Chapovalov Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#42745} --- video/frame_cadence_adapter.cc | 34 ++++++++++++++++++++----- video/frame_cadence_adapter_unittest.cc | 31 +++++++++++++++++++++- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/video/frame_cadence_adapter.cc b/video/frame_cadence_adapter.cc index b11c7ba913..8a7852b369 100644 --- a/video/frame_cadence_adapter.cc +++ b/video/frame_cadence_adapter.cc @@ -279,6 +279,11 @@ class VSyncEncodeAdapterMode : public AdapterMode { worker_sequence_checker_.Detach(); } + void PrepareShutdown() { + MutexLock lock(&queue_lock_); + queue_ = nullptr; + } + // Adapter overrides. void OnFrame(Timestamp post_time, bool queue_overload, @@ -309,7 +314,12 @@ class VSyncEncodeAdapterMode : public AdapterMode { }; Clock* const clock_; - TaskQueueBase* queue_; + // Protects `queue_`. + // TODO: crbug.com/358040973 - We should eventually figure out a way to avoid + // lock protection. + Mutex queue_lock_; + TaskQueueBase* queue_ RTC_GUARDED_BY(queue_lock_) + RTC_PT_GUARDED_BY(queue_lock_); RTC_NO_UNIQUE_ADDRESS SequenceChecker queue_sequence_checker_; rtc::scoped_refptr queue_safety_flag_; // Input frame rate statistics for use when not in zero-hertz mode. @@ -826,12 +836,23 @@ void VSyncEncodeAdapterMode::EncodeAllEnqueuedFrames() { (post_time - input.time_when_posted_us).ms()); const VideoFrame frame = std::move(input.video_frame); - queue_->PostTask(SafeTask(queue_safety_flag_, [this, post_time, frame] { - RTC_DCHECK_RUN_ON(queue_); + MutexLock lock(&queue_lock_); + if (queue_) { + queue_->PostTask(SafeTask(queue_safety_flag_, [this, post_time, frame] { + { + MutexLock lock(&queue_lock_); + if (!queue_) { + return; + } + RTC_DCHECK_RUN_ON(queue_); + } - // TODO(b/304158952): Support more refined queue overload control. - callback_->OnFrame(post_time, /*queue_overload=*/false, frame); - })); + // TODO(b/304158952): Support more refined queue overload control. + // Not running under mutex is safe since `callback_` existence is + // guaranteed to exist as long as running encode queue tasks exist. + callback_->OnFrame(post_time, /*queue_overload=*/false, frame); + })); + } } input_queue_.clear(); @@ -858,6 +879,7 @@ FrameCadenceAdapterImpl::~FrameCadenceAdapterImpl() { // VSync adapter needs to be destroyed on worker queue when metronome is // valid. if (metronome_) { + vsync_encode_adapter_->PrepareShutdown(); absl::Cleanup cleanup = [adapter = std::move(vsync_encode_adapter_)] {}; worker_queue_->PostTask([cleanup = std::move(cleanup)] {}); } diff --git a/video/frame_cadence_adapter_unittest.cc b/video/frame_cadence_adapter_unittest.cc index d46434bed3..237b14299e 100644 --- a/video/frame_cadence_adapter_unittest.cc +++ b/video/frame_cadence_adapter_unittest.cc @@ -27,6 +27,7 @@ #include "rtc_base/event.h" #include "rtc_base/logging.h" #include "rtc_base/rate_statistics.h" +#include "rtc_base/task_queue_for_test.h" #include "rtc_base/time_utils.h" #include "system_wrappers/include/metrics.h" #include "system_wrappers/include/ntp_time.h" @@ -579,7 +580,7 @@ TEST(FrameCadenceAdapterTest, EncodeFramesAreAlignedWithMetronomeTick) { "queue", TaskQueueFactory::Priority::NORMAL); auto worker_queue = time_controller.GetTaskQueueFactory()->CreateTaskQueue( "work_queue", TaskQueueFactory::Priority::NORMAL); - static test::FakeMetronome metronome(kTickPeriod); + test::FakeMetronome metronome(kTickPeriod); test::ScopedKeyValueConfig no_field_trials; auto adapter = FrameCadenceAdapterInterface::Create( time_controller.GetClock(), queue.get(), &metronome, worker_queue.get(), @@ -646,6 +647,34 @@ TEST(FrameCadenceAdapterTest, EncodeFramesAreAlignedWithMetronomeTick) { finalized.Wait(rtc::Event::kForever); } +TEST(FrameCadenceAdapterTest, ShutdownUnderMetronome) { + // Regression test for crbug.com/356423094. + // The current thread takes the role of worker queue. + GlobalSimulatedTimeController time_controller(Timestamp::Zero()); + static constexpr TimeDelta kTickPeriod = TimeDelta::Millis(100); + auto queue = time_controller.GetTaskQueueFactory()->CreateTaskQueue( + "queue", TaskQueueFactory::Priority::NORMAL); + test::FakeMetronome metronome(kTickPeriod); + test::ScopedKeyValueConfig no_field_trials; + auto adapter = FrameCadenceAdapterInterface::Create( + time_controller.GetClock(), queue.get(), &metronome, + TaskQueueBase::Current(), no_field_trials); + MockCallback callback; + EXPECT_CALL(callback, OnFrame).Times(0); + adapter->Initialize(&callback); + + // Pass a frame, this is expected to trigger an encode call in the future. + adapter->OnFrame(CreateFrame()); + + // Then post destruction of the adapter and destroy the encode queue from the + // worker (i.e. current). + SendTask(queue.get(), [&] { adapter = nullptr; }); + queue = nullptr; + + // Now that we advance time, there should be no encoding happening. + time_controller.AdvanceTime(TimeDelta::Millis(100)); +} + class FrameCadenceAdapterSimulcastLayersParamTest : public ::testing::TestWithParam { public: