From 4abcf095db62d0930df89640162b447264a29546 Mon Sep 17 00:00:00 2001 From: Alexei Pastuchov Date: Fri, 24 Jul 2026 18:20:37 +0200 Subject: [PATCH] Fix data races in connection lifecycle code (TSAN findings, #460) is_dead/io_list/proc_list/proc_removed/to_be_freed_list are set on one thread and polled on another (IO/proc/main) with no synchronization, so make them std::atomic. Also take con->thread->lock when reading the io_packet_list head in _thread_packet_flush(), since the FIFO is appended to under that lock by other threads, and drop the unlocked pre-check in gearman_server_con_to_be_freed_next() that read thread->to_be_freed_list before acquiring the same lock the writer uses. Verified under -fsanitize=thread with a --threads=4 gearmand instance hammered by 16 concurrent clients (4800 background job submissions, no worker): none of the previously reported races at these sites reproduce anymore. Signed-off-by: Alexei Pastuchov --- libgearman-server/connection.cc | 8 +++---- libgearman-server/struct/io.h | 17 +++++++++----- libgearman-server/thread.cc | 39 ++++++++++++++++++++++++++++----- 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/libgearman-server/connection.cc b/libgearman-server/connection.cc index bd7394e09..b58237c1d 100644 --- a/libgearman-server/connection.cc +++ b/libgearman-server/connection.cc @@ -427,11 +427,9 @@ gearman_server_con_st * gearman_server_con_to_be_freed_next(gearman_server_threa { gearman_server_con_st *con; - if (thread->to_be_freed_list == NULL) - { - return NULL; - } - + /* thread->to_be_freed_list is mutated under thread->lock by other threads + (e.g. gearman_server_con_to_be_freed_add()), so it must not be read + before taking the lock below. */ int lock_error; if ((lock_error= pthread_mutex_lock(&thread->lock)) == 0) { diff --git a/libgearman-server/struct/io.h b/libgearman-server/struct/io.h index d2316a6b4..aef61af60 100644 --- a/libgearman-server/struct/io.h +++ b/libgearman-server/struct/io.h @@ -41,6 +41,8 @@ #include "libgearman/ssl.h" +#include + struct gearmand_io_st { struct { @@ -146,14 +148,19 @@ struct gearman_server_con_st gearmand_io_st con; bool is_sleeping{}; bool is_exceptions{}; - bool is_dead{}; + // These flags are used to hand connection ownership off between the IO, + // proc, and main threads (e.g. gearman_server_con_attempt_free() sets + // is_dead from an IO thread while _proc() reads it from the proc thread + // with no other synchronization), so they must be atomic rather than + // plain bool. + std::atomic is_dead{false}; bool is_noop_sent{}; bool is_cleaned_up{}; gearmand_error_t ret{}; - bool io_list{}; - bool proc_list{}; - bool proc_removed{}; - bool to_be_freed_list{}; + std::atomic io_list{false}; + std::atomic proc_list{false}; + std::atomic proc_removed{false}; + std::atomic to_be_freed_list{false}; uint32_t io_packet_count{}; uint32_t proc_packet_count{}; uint32_t worker_count{}; diff --git a/libgearman-server/thread.cc b/libgearman-server/thread.cc index 247611719..0fd4d54e4 100644 --- a/libgearman-server/thread.cc +++ b/libgearman-server/thread.cc @@ -189,7 +189,7 @@ gearman_server_thread_run(gearman_server_thread_st *thread, gearman_server_con_free(server_con); else gearmand_log_error(GEARMAN_DEFAULT_LOG_PARAM, "con %p isn't dead %d or proc removed %d, but is in to_be_freed_list", - server_con, server_con->is_dead, server_con->proc_removed); + server_con, bool(server_con->is_dead), bool(server_con->proc_removed)); } while ((server_con= gearman_server_con_io_next(thread)) != NULL) @@ -344,18 +344,45 @@ static gearmand_error_t _thread_packet_flush(gearman_server_con_st *con) return GEARMAND_IO_WAIT; } - while (con->io_packet_list) + while (1) { - gearmand_error_t ret= gearman_io_send(con, &(con->io_packet_list->packet), - con->io_packet_list->next == NULL ? true : false); + /* con->io_packet_list is appended to under con->thread->lock by other + threads (e.g. gearman_server_io_packet_add() from the proc thread), so + the head pointer and its ->next must be read under the same lock + rather than dereferenced directly here. */ + gearman_server_packet_st *packet; + bool is_last; + + int lock_error; + if ((lock_error= pthread_mutex_lock(&con->thread->lock)) == 0) + { + packet= con->io_packet_list; + is_last= (packet != NULL) and (packet->next == NULL); + if ((lock_error= pthread_mutex_unlock(&con->thread->lock))) + { + gearmand_log_fatal_perror(GEARMAN_DEFAULT_LOG_PARAM, lock_error, "pthread_mutex_unlock"); + } + } + else + { + gearmand_log_fatal_perror(GEARMAN_DEFAULT_LOG_PARAM, lock_error, "pthread_mutex_lock"); + return GEARMAND_ERRNO; + } + + if (packet == NULL) + { + break; + } + + gearmand_error_t ret= gearman_io_send(con, &(packet->packet), is_last); if (gearmand_failed(ret)) { return ret; } - gearmand_log_debug(GEARMAN_DEFAULT_LOG_PARAM, + gearmand_log_debug(GEARMAN_DEFAULT_LOG_PARAM, "Sent %s", - gearman_strcommand(con->io_packet_list->packet.command)); + gearman_strcommand(packet->packet.command)); gearman_server_io_packet_remove(con); }