From ea8a3b3b10177d5570d875cb32d5b36e5e714d54 Mon Sep 17 00:00:00 2001 From: Olivier Dion <olivier.dion@polymtl.ca> Date: Tue, 11 May 2021 16:27:23 -0400 Subject: [PATCH] scheduled_executor: Fix deadlock If the scheduler's has a job that owns the last reference to the scheduler itself, then the thread will try to join itself in the destructor of the scheduler, resulting in `std::terminate()`. Thus, detach the thread if the scheduler is freed from its thread, because nobody is supposed to have a reference to the thread. This introduces a read after free of `running_`. By making it a shared value, the thread can keep a reference after destruction of the scheduler. Gitlab: #545 Change-Id: I0118c99a5718d302f916099e24606add444546d8 --- src/scheduled_executor.cpp | 25 +++++++++++++++++++------ src/scheduled_executor.h | 2 +- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/scheduled_executor.cpp b/src/scheduled_executor.cpp index f33f996b28..e3566007c2 100644 --- a/src/scheduled_executor.cpp +++ b/src/scheduled_executor.cpp @@ -23,8 +23,12 @@ namespace jami { ScheduledExecutor::ScheduledExecutor() - : thread_([this] { - while (running_.load()) + : running_(std::make_shared<std::atomic<bool>>(true)) + , thread_([this, is_running = running_] { + // The thread needs its own reference of `running_` in case the + // scheduler is destroyed within the thread because of a job + + while (*is_running) loop(); }) {} @@ -32,8 +36,17 @@ ScheduledExecutor::ScheduledExecutor() ScheduledExecutor::~ScheduledExecutor() { stop(); - if (thread_.joinable()) + + if (not thread_.joinable()) { + return; + } + + // Avoid deadlock + if (std::this_thread::get_id() == thread_.get_id()) { + thread_.detach(); + } else { thread_.join(); + } } void @@ -41,7 +54,7 @@ ScheduledExecutor::stop() { { std::lock_guard<std::mutex> lock(jobLock_); - running_ = false; + *running_ = false; jobs_.clear(); } cv_.notify_all(); @@ -106,7 +119,7 @@ ScheduledExecutor::loop() std::vector<Job> jobs; { std::unique_lock<std::mutex> lock(jobLock_); - while (running_ and (jobs_.empty() or jobs_.begin()->first > clock::now())) { + while (*running_ and (jobs_.empty() or jobs_.begin()->first > clock::now())) { if (jobs_.empty()) cv_.wait(lock); else { @@ -114,7 +127,7 @@ ScheduledExecutor::loop() cv_.wait_until(lock, nextJob); } } - if (not running_) + if (not *running_) return; jobs = std::move(jobs_.begin()->second); jobs_.erase(jobs_.begin()); diff --git a/src/scheduled_executor.h b/src/scheduled_executor.h index b65a56d63c..532c4d9f10 100644 --- a/src/scheduled_executor.h +++ b/src/scheduled_executor.h @@ -137,7 +137,7 @@ private: void schedule(std::shared_ptr<Task>, time_point t); void reschedule(std::shared_ptr<RepeatedTask>, time_point t, duration dt); - std::atomic_bool running_ {true}; + std::shared_ptr<std::atomic<bool>> running_; std::map<time_point, std::vector<Job>> jobs_ {}; std::mutex jobLock_ {}; std::condition_variable cv_ {}; -- GitLab