Data races in connection lifecycle code (found via TSAN while validating PR #459)

[p-alik]

Summary

While validating PR #459 under ThreadSanitizer (building with -fsanitize=thread and stress-testing a --threads=4 gearmand instance), TSAN reported a cluster of data races in the connection lifecycle code. These are not related to PR #459's changes — they reproduce independent of the epoch-job feature, purely from concurrent client connections and normal job traffic across the proc/IO/main threads. Filing separately since they're pre-existing.

How this was found

• Built gearmand with CXXFLAGS='-fsanitize=thread -g -O1' LDFLAGS='-fsanitize=thread', --threads=4.
• Drove load with a client hammering the server from 16 threads, ~5k submissions per run, over several runs (~50k total).
• No worker was needed to trigger these — they show up purely from connection setup/teardown and packet queuing across the proc thread, IO threads, and the main thread.

Representative races

1. con->is_dead read/written with no lock at all

Write of size 1 at ... by thread T4 (IO thread):
  #0 gearman_server_con_attempt_free connection.cc:188
     con->is_dead= true;
  #1 gearmand_con_free gearmand_con.cc:721

Previous read of size 1 at ... by thread T3 (proc thread):
  #0 _proc gearmand_con.cc:515
     if (con->is_dead)

libgearman-server/connection.cc:188 sets con->is_dead from an IO thread with no lock; libgearman-server/gearmand_con.cc:515 reads it from the proc thread, also with no lock.

2. con->io_packet_list FIFO: writer locks, reader doesn't

Write of size 8 at ... by thread T3 (proc thread, mutexes: write M0 = con->thread->lock):
  #0 gearman_server_io_packet_add packet.cc:193
     GEARMAND_FIFO__ADD(con->io_packet, server_packet);

Previous read of size 8 at ... by thread T5 (IO thread, no mutex held):
  #0 _thread_packet_flush thread.cc:350
     while (con->io_packet_list)

gearman_server_io_packet_add() (libgearman-server/packet.cc:193) correctly takes con->thread->lock before touching the FIFO, but _thread_packet_flush() (libgearman-server/thread.cc:350) reads con->io_packet_list without taking that same lock.

3. thread->to_be_freed_list unlocked fast-path check before the lock

gearman_server_con_to_be_freed_next() (libgearman-server/connection.cc:430) does:

if (thread->to_be_freed_list == NULL)   // <- read outside the lock
{
  return NULL;
}
int lock_error;
if ((lock_error= pthread_mutex_lock(&thread->lock)) == 0)  // lock only taken after the check

while gearman_server_con_to_be_freed_add() (libgearman-server/connection.cc:403) always mutates to_be_freed_list under thread->lock. The unlocked pre-check in _next() races against the locked writer.

Other race sites observed (same session, same pattern of missing/inconsistent locking)

• libgearman-server/connection.cc:189 (gearman_server_con_attempt_free)
• libgearman-server/connection.cc:477 (gearman_server_con_io_add)
• libgearman-server/connection.cc:559 (gearman_server_con_proc_add)
• libgearman-server/connection.cc:614, :628 (gearman_server_con_proc_next)
• libgearman-server/gearmand_con.cc:504 (_proc)
• libgearman-server/gearmand_con.cc:633, :634, :636 (gearmand_con_create)
• libgearman-server/gearmand_con.cc:761 (gearmand_con_check_queue)
• libgearman-server/io.cc:592, :643 (gearman_io_send)
• libgearman-server/packet.cc:238 (gearman_server_proc_packet_add)
• libgearman-server/plugins/protocol/gear/protocol.cc:324, :336 (Geartext::pack)
• libgearman-server/thread.cc:203 (gearman_server_thread_run)
• libgearman-server/thread.cc:356 (_thread_packet_flush)
• a memcpy race under bits/string_fortified.h:29, called from the packet/protocol path

44 individual TSAN reports total across these sites in one test session.

Scope / what's untested

• Reproduced with --threads=4. I did not test whether these also reproduce with the default --threads=0 — worth checking, since the proc thread runs as a separate OS thread from the IO/main thread even at --threads=0, so some of these (particularly the proc-thread-vs-IO-thread ones) may not be specific to --threads>0.
• No crash was observed during these runs; these are TSAN-detected memory-ordering violations, not confirmed-reproducible corruption. Severity/exploitability would need further investigation — flagging so a maintainer familiar with the connection lifecycle code can judge which of these are real risk vs. currently-benign due to scheduling luck.

Repro

CXXFLAGS='-fsanitize=thread -g -O1' CFLAGS='-fsanitize=thread -g -O1' LDFLAGS='-fsanitize=thread' ./configure ...
make
TSAN_OPTIONS='halt_on_error=0:log_path=/tmp/tsan' ./gearmand/gearmand --threads=4 --verbose=INFO &
# then drive concurrent client connections / job submissions against it

[esabol]

Wow. That's not good.

You might want to include SSL/TLS into your testing. I've long suspected that a mutex is needed there whenever reading or writing bytes to the encrypted connection (because the code for a different server daemon that supports SSL/TLS connections did that and the code comments claimed it was needed). In practice, I have very rarely noticed a problem, but we don't have a large number and frequency of jobs hammering on our gearmand server.

[esabol]

@p-alik : Do you think fixes for any of these things are needed before 2.0.0 is released?

[p-alik]

@esabol, I'm reluctant to pack more changes into 2.0.0. But if you want this resolved before the release, I can give it a try.

[esabol]

I'm OK with deferring to a later release. I imagine fixing this stuff would entail extensive changes, and gearmand has worked well enough without fixing these races for many years.