threadpool: guard against Start-Stop race
What the developer wrote
Authored by furszy
threadpool: guard against Start-Stop race
Stop() has two windows where Start() could cause troubles:
1) m_workers is temporarily empty while workers are being joined,
this creates a window where Start() could slip through and reset
m_interrupt to false, preventing the old workers from exiting and
causing a deadlock.
2) Start() could be called after workers are joined but before the
empty() sanity check on m_work_queue, causing a crash.
Fix both races by keeping m_interrupt set for the entire duration
of Stop(), so any concurrent Start() call is rejected until all
workers have exited.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
What changed, and why it matters
This patch fixes a timing bug in Bitcoin Core's internal worker-thread pool. If someone started the thread pool while it was still stopping, the program could freeze (deadlock) or crash. The fix keeps the pool marked as 'stopping' until every worker thread has fully exited, so a new start request is rejected during that window.
Review and merge. The patch is small, targeted, and the commit message clearly explains the race being fixed. No additional action is required unless downstream users rely on being able to call Start() during Stop(), which the patch intentionally prohibits.
Security signals we found
Race condition between Start() and Stop() in thread pool lifecycle
Potential deadlock when m_interrupt is reset while workers are being joined
Potential crash from Start() before m_work_queue empty sanity check
Fix uses existing m_interrupt flag to reject concurrent Start() during Stop()
Evidence from the diff
The change addresses two race windows in util/threadpool.h between Start() and Stop(). Previously, Stop() could join workers while m_interrupt was temporarily false, allowing Start() to reset m_interrupt and re-create workers, which could deadlock with the old join loop or crash because m_work_queue was not yet verified empty. The patch moves m_interrupt = false to the end of Stop() after all workers are joined and the queue-empty sanity check is done, and Start() now rejects calls whenever m_interrupt is true. This serializes Start() after Stop() completes.
Changed components
src/util/threadpool.hThreadPool::Start()ThreadPool::Stop()Inspect captured patch +11 / −3
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
index c039b59c..6fc29498 100644
--- a/src/util/threadpool.h
+++ b/src/util/threadpool.h
@@ -105,8 +105,8 @@ public:
{
assert(num_workers > 0);
LOCK(m_mutex);
+ if (m_interrupt) throw std::runtime_error("Thread pool has been interrupted or is stopping");
if (!m_workers.empty()) throw std::runtime_error("Thread pool already started");
- m_interrupt = false; // Reset
// Create workers
m_workers.reserve(num_workers);
@@ -122,6 +122,7 @@ public:
* Any remaining tasks in the queue will be processed before returning.
*
* Must be called from a controller (non-worker) thread.
+ * Concurrent calls to Start() will be rejected while Stop() is in progress.
*/
void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
@@ -139,9 +140,12 @@ public:
}
m_cv.notify_all();
for (auto& worker : threads_to_join) worker.join();
+
// Since we currently wait for tasks completion, sanity-check empty queue
- WITH_LOCK(m_mutex, Assume(m_work_queue.empty()));
- // Note: m_interrupt is left true until next Start()
+ LOCK(m_mutex);
+ Assume(m_work_queue.empty());
+ // Re-allow Start() now that all workers have exited
+ m_interrupt = false;
}
enum class SubmitError {
@@ -202,6 +206,10 @@ public:
*
* Wakes all worker threads so they can drain the queue and exit.
* Unlike Stop(), this function does not wait for threads to finish.
+ *
+ * Note: The next step in the pool lifecycle is calling Stop(), which
+ * releases any dangling resources and resets the pool state
+ * for shutdown or restart.
*/
void Interrupt() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
Why this scored 42/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.