From 7654081e9153f0670b17e304aef14f7ac997b523 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Thu, 11 Apr 2019 17:48:30 +0200 Subject: [PATCH] Fix for use after free in simulated time controller. We need to keep the lock until we have finished using the task runner returned by GetNextReadyRunner to ensure that we don't remove it from another thread. Additionally, we must get only one runner at a time in case the first runner removes the second runner. Bug: webrtc:10538 Change-Id: Idbd5610b67666238545b3a321fb79f7e86fcac56 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132342 Reviewed-by: Niels Moller Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#27584} --- .../simulated_time_controller.cc | 33 ++++++++++--------- .../simulated_time_controller.h | 9 ++--- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/test/time_controller/simulated_time_controller.cc b/test/time_controller/simulated_time_controller.cc index 10592edaf4..ba5e162cf0 100644 --- a/test/time_controller/simulated_time_controller.cc +++ b/test/time_controller/simulated_time_controller.cc @@ -312,18 +312,6 @@ std::unique_ptr SimulatedTimeControllerImpl::CreateProcessThread( return process_thread; } -std::vector -SimulatedTimeControllerImpl::GetNextReadyRunner(Timestamp current_time) { - rtc::CritScope lock(&lock_); - std::vector ready; - for (auto* runner : runners_) { - if (yielded_.find(runner) == yielded_.end() && - runner->GetNextRunTime() <= current_time) { - ready.push_back(runner); - } - } - return ready; -} void SimulatedTimeControllerImpl::YieldExecution() { if (rtc::CurrentThreadId() == thread_id_) { @@ -352,11 +340,23 @@ void SimulatedTimeControllerImpl::RunReadyRunners() { // We repeat until we have no ready left to handle tasks posted by ready // runners. while (true) { - auto ready = GetNextReadyRunner(current_time); - if (ready.empty()) - break; - for (auto* runner : ready) { + rtc::CritScope lock(&lock_); + RTC_DCHECK(ready_runners_.empty()); + for (auto* runner : runners_) { + if (yielded_.find(runner) == yielded_.end() && + runner->GetNextRunTime() <= current_time) { + ready_runners_.push_back(runner); + } + } + if (ready_runners_.empty()) + return; + while (!ready_runners_.empty()) { + auto* runner = ready_runners_.front(); + ready_runners_.pop_front(); runner->UpdateReady(current_time); + // Note that the Run function might indirectly cause a call to + // Unregister() which will recursively grab |lock_| again to remove items + // from |ready_runners_|. runner->Run(current_time); } } @@ -390,6 +390,7 @@ void SimulatedTimeControllerImpl::Unregister(SimulatedSequenceRunner* runner) { rtc::CritScope lock(&lock_); bool removed = RemoveByValue(runners_, runner); RTC_CHECK(removed); + RemoveByValue(ready_runners_, runner); } } // namespace sim_time_impl diff --git a/test/time_controller/simulated_time_controller.h b/test/time_controller/simulated_time_controller.h index 6837643412..b870fa353d 100644 --- a/test/time_controller/simulated_time_controller.h +++ b/test/time_controller/simulated_time_controller.h @@ -60,16 +60,17 @@ class SimulatedTimeControllerImpl : public TaskQueueFactory, void Unregister(SimulatedSequenceRunner* runner); private: - // Returns runners in |runners_| that are ready for execution. - std::vector GetNextReadyRunner( - Timestamp current_time) RTC_RUN_ON(thread_checker_); - const rtc::PlatformThreadId thread_id_; rtc::ThreadChecker thread_checker_; rtc::CriticalSection time_lock_; Timestamp current_time_ RTC_GUARDED_BY(time_lock_); rtc::CriticalSection lock_; std::vector runners_ RTC_GUARDED_BY(lock_); + // Used in RunReadyRunners() to keep track of ready runners that are to be + // processed in a round robin fashion. the reason it's a member is so that + // runners can removed from here by Unregister(). + std::list ready_runners_ RTC_GUARDED_BY(lock_); + // Task queues on which YieldExecution has been called. std::unordered_set yielded_ RTC_GUARDED_BY(thread_checker_); };