From 4f46a354e67f4a7781570f6f4e17738eeca9d5ac Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Mon, 23 Mar 2020 09:27:28 +0100 Subject: [PATCH] BUG/MINOR: haproxy/threads: close a possible race in soft-stop detection Commit 4b3f27b ("BUG/MINOR: haproxy/threads: try to make all threads leave together") improved the soft-stop synchronization but it left a small race open because it looks at tasks_run_queue, which can drop to zero then back to one while another thread picks the task from the run queue to insert it into the tasklet_list. The risk is very low but not null. In addition the condition didn't consider the possible presence of signals in the queue. This patch moves the stopping detection just after the "wake" calculation which already takes care of the various queues' sizes and signals. It avoids needlessly duplicating these tests. The bug was discovered during a code review but will probably never be observed. This fix may be backported to 2.1 and 2.0 along with the commit above. --- src/haproxy.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/haproxy.c b/src/haproxy.c index 4feee95d4..ca0ab3270 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -2806,14 +2806,6 @@ void run_poll_loop() if (tid == 0) signal_process_queue(); - if (stopping && tasks_run_queue == 0) - _HA_ATOMIC_OR(&stopping_thread_mask, tid_bit); - - /* stop when there's nothing left to do */ - if ((jobs - unstoppable_jobs) == 0 && tasks_run_queue == 0 && - (stopping_thread_mask & all_threads_mask) == all_threads_mask) - break; - /* also stop if we failed to cleanly stop all tasks */ if (killed > 1) break; @@ -2834,6 +2826,16 @@ void run_poll_loop() wake = 0; } + if (!wake) { + if (stopping) + _HA_ATOMIC_OR(&stopping_thread_mask, tid_bit); + + /* stop when there's nothing left to do */ + if ((jobs - unstoppable_jobs) == 0 && + (stopping_thread_mask & all_threads_mask) == all_threads_mask) + break; + } + /* If we have to sleep, measure how long */ next = wake ? TICK_ETERNITY : next_timer_expiry();