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 <nisse@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27584}diff --git a/test/time_controller/simulated_time_controller.cc b/test/time_controller/simulated_time_controller.cc
index 10592ed..ba5e162 100644
--- a/test/time_controller/simulated_time_controller.cc
+++ b/test/time_controller/simulated_time_controller.cc
@@ -312,18 +312,6 @@
return process_thread;
}
-std::vector<SimulatedSequenceRunner*>
-SimulatedTimeControllerImpl::GetNextReadyRunner(Timestamp current_time) {
- rtc::CritScope lock(&lock_);
- std::vector<SimulatedSequenceRunner*> 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 @@
// 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 @@
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 6837643..b870fa3 100644
--- a/test/time_controller/simulated_time_controller.h
+++ b/test/time_controller/simulated_time_controller.h
@@ -60,16 +60,17 @@
void Unregister(SimulatedSequenceRunner* runner);
private:
- // Returns runners in |runners_| that are ready for execution.
- std::vector<SimulatedSequenceRunner*> 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<SimulatedSequenceRunner*> 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<SimulatedSequenceRunner*> ready_runners_ RTC_GUARDED_BY(lock_);
+
// Task queues on which YieldExecution has been called.
std::unordered_set<TaskQueueBase*> yielded_ RTC_GUARDED_BY(thread_checker_);
};
