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 <handellm@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42745}

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<PendingTaskSafetyFlag> queue_safety_flag_;
   // Input frame rate statistics for use when not in zero-hertz mode.
@@ -826,13 +836,24 @@ void VSyncEncodeAdapterMode::EncodeAllEnqueuedFrames() {
                  (post_time - input.time_when_posted_us).ms());
 
     const VideoFrame frame = std::move(input.video_frame);
+    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.
+        // 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)] {});
   }

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<int> {
  public: