From: Noel Power Date: Wed, 12 Nov 2025 12:24:59 +0000 (+0000) Subject: lib/pthreadpool: Fix possible concurrent access to pool->glue_list X-Git-Tag: tdb-1.4.15~49 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=07725914a5af02af3b4dc267b76b531bf49254e8;p=thirdparty%2Fsamba.git lib/pthreadpool: Fix possible concurrent access to pool->glue_list ThreadSanitizer run against tests added in previous commit identify a race condition with pool->glue_list with concurrent thread access WARNING: ThreadSanitizer: data race (pid=13574) Read of size 8 at 0x7b2000000368 by thread T7: #0 pthreadpool_tevent_job_signal ../../lib/pthreadpool/pthreadpool_tevent.c:370 (pthreadpool_tevent_unit_test_san+0x406c6e) #1 pthreadpool_server ../../lib/pthreadpool/pthreadpool.c:657 (pthreadpool_tevent_unit_test_san+0x40443b) #2 (libtsan.so.0+0x323cf) Previous write of size 8 at 0x7b2000000368 by main thread: #0 pthreadpool_tevent_glue_destructor ../../lib/pthreadpool/pthreadpool_tevent.c:165 (pthreadpool_tevent_unit_test_san+0x405aed) #1 _tc_free_internal ../../lib/talloc/talloc.c:1158 (libtalloc-private-samba.so+0x3419) #2 _tc_free_internal ../../lib/talloc/talloc.c:1158 (libtalloc-private-samba.so+0x3419) #3 cmocka_run_one_test_or_fixture ../../third_party/cmocka/cmocka.c:2948 (libcmocka-private-samba.so+0x6f92) #4 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x40e6b) Location is heap block of size 120 at 0x7b2000000300 allocated by main thread: #0 malloc (libtsan.so.0+0x35799) #1 __talloc_with_prefix ../../lib/talloc/talloc.c:783 (libtalloc-private-samba.so+0x2a99) #2 test_pthreadpool_tevent_job_send_multiple_2 ../../lib/pthreadpool/test_pthreadpool_tevent.c:399 (pthreadpool_tevent_unit_test_san+0x40856f) #3 cmocka_run_one_test_or_fixture ../../third_party/cmocka/cmocka.c:2948 (libcmocka-private-samba.so+0x6f92) #4 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x40e6b) Thread T7 (tid=13582, running) created by main thread at: #0 pthread_create (libtsan.so.0+0x5ed75) #1 pthreadpool_create_thread ../../lib/pthreadpool/pthreadpool.c:711 (pthreadpool_tevent_unit_test_san+0x4045ac) #2 pthreadpool_add_job ../../lib/pthreadpool/pthreadpool.c:792 (pthreadpool_tevent_unit_test_san+0x40496f) #3 pthreadpool_tevent_job_send ../../lib/pthreadpool/pthreadpool_tevent.c:329 (pthreadpool_tevent_unit_test_san+0x4065e2) #4 test_pthreadpool_tevent_job_send_multiple_2 ../../lib/pthreadpool/test_pthreadpool_tevent.c:423 (pthreadpool_tevent_unit_test_san+0x4086b2) #5 cmocka_run_one_test_or_fixture ../../third_party/cmocka/cmocka.c:2948 (libcmocka-private-samba.so+0x6f92) #6 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x40e6b) SUMMARY: ThreadSanitizer: data race ../../lib/pthreadpool/pthreadpool_tevent.c:370 in pthreadpool_tevent_job_signal BUG: https://bugzilla.samba.org/show_bug.cgi?id=15958 Signed-off-by: Noel Power Reviewed-by: Stefan Metzmacher --- diff --git a/lib/pthreadpool/pthreadpool_tevent.c b/lib/pthreadpool/pthreadpool_tevent.c index 515dcc0fd7b..19cf2dfbc15 100644 --- a/lib/pthreadpool/pthreadpool_tevent.c +++ b/lib/pthreadpool/pthreadpool_tevent.c @@ -19,6 +19,7 @@ #include "replace.h" #include "system/filesys.h" +#include "system/threads.h" #include "pthreadpool_tevent.h" #include "pthreadpool.h" #include "lib/util/tevent_unix.h" @@ -57,6 +58,10 @@ struct pthreadpool_tevent_glue_ev_link { struct pthreadpool_tevent { struct pthreadpool *pool; struct pthreadpool_tevent_glue *glue_list; + /* + * Control access to the glue_list + */ + pthread_mutex_t glue_mutex; struct pthreadpool_tevent_job_state *jobs; }; @@ -97,6 +102,12 @@ int pthreadpool_tevent_init(TALLOC_CTX *mem_ctx, unsigned max_threads, return ret; } + ret = pthread_mutex_init(&pool->glue_mutex, NULL); + if (ret != 0) { + TALLOC_FREE(pool); + return ret; + } + talloc_set_destructor(pool, pthreadpool_tevent_destructor); *presult = pool; @@ -138,6 +149,11 @@ static int pthreadpool_tevent_destructor(struct pthreadpool_tevent *pool) state->pool = NULL; } + ret = pthread_mutex_lock(&pool->glue_mutex); + if (ret != 0) { + return ret; + } + /* * Delete all the registered * tevent_context/tevent_threaded_context @@ -147,6 +163,10 @@ static int pthreadpool_tevent_destructor(struct pthreadpool_tevent *pool) /* The glue destructor removes it from the list */ TALLOC_FREE(glue); } + + pthread_mutex_unlock(&pool->glue_mutex); + pthread_mutex_destroy(&pool->glue_mutex); + pool->glue_list = NULL; ret = pthreadpool_destroy(pool->pool); @@ -158,6 +178,16 @@ static int pthreadpool_tevent_destructor(struct pthreadpool_tevent *pool) return 0; } +/* + * glue destruction is only called with + * glue_mutex already locked either from + * a) pthreadpool_tevent_destructor or + * b) pthreadpool_tevent_glue_link_destructor + * pthreadpool_tevent_destructor accesses + * the glue_list while calling pthreadpool_tevent_glue_destructor + * which modifies the glue_list so it needs the lock held while + * doing that. + */ static int pthreadpool_tevent_glue_destructor( struct pthreadpool_tevent_glue *glue) { @@ -190,7 +220,21 @@ static int pthreadpool_tevent_glue_destructor( static int pthreadpool_tevent_glue_link_destructor( struct pthreadpool_tevent_glue_ev_link *ev_link) { - TALLOC_FREE(ev_link->glue); + if (ev_link->glue) { + int ret; + /* save the mutex */ + pthread_mutex_t *glue_mutex = + &ev_link->glue->pool->glue_mutex; + ret = pthread_mutex_lock(glue_mutex); + if (ret != 0) { + return ret; + } + TALLOC_FREE(ev_link->glue); + ret = pthread_mutex_unlock(glue_mutex); + if (ret != 0) { + return ret; + } + } return 0; } @@ -199,7 +243,12 @@ static int pthreadpool_tevent_register_ev(struct pthreadpool_tevent *pool, { struct pthreadpool_tevent_glue *glue = NULL; struct pthreadpool_tevent_glue_ev_link *ev_link = NULL; + int ret; + ret = pthread_mutex_lock(&pool->glue_mutex); + if (ret != 0) { + return ret; + } /* * See if this tevent_context was already registered by * searching the glue object list. If so we have nothing @@ -208,10 +257,18 @@ static int pthreadpool_tevent_register_ev(struct pthreadpool_tevent *pool, */ for (glue = pool->glue_list; glue != NULL; glue = glue->next) { if (glue->ev == ev) { + ret = pthread_mutex_unlock(&pool->glue_mutex); + if (ret != 0) { + return ret; + } return 0; } } + ret = pthread_mutex_unlock(&pool->glue_mutex); + if (ret != 0) { + return ret; + } /* * Event context not yet registered - create a new glue * object containing a tevent_context/tevent_threaded_context @@ -254,7 +311,17 @@ static int pthreadpool_tevent_register_ev(struct pthreadpool_tevent *pool, } #endif + ret = pthread_mutex_lock(&pool->glue_mutex); + if (ret != 0) { + return ret; + } + DLIST_ADD(pool->glue_list, glue); + + ret = pthread_mutex_unlock(&pool->glue_mutex); + if (ret != 0) { + return ret; + } return 0; } @@ -359,6 +426,7 @@ static int pthreadpool_tevent_job_signal(int jobid, job_private_data, struct pthreadpool_tevent_job_state); struct tevent_threaded_context *tctx = NULL; struct pthreadpool_tevent_glue *g = NULL; + int ret; if (state->pool == NULL) { /* The pthreadpool_tevent is already gone */ @@ -366,6 +434,11 @@ static int pthreadpool_tevent_job_signal(int jobid, } #ifdef HAVE_PTHREAD + ret = pthread_mutex_lock(&state->pool->glue_mutex); + if (ret != 0) { + return ret; + } + for (g = state->pool->glue_list; g != NULL; g = g->next) { if (g->ev == state->ev) { tctx = g->tctx; @@ -373,6 +446,11 @@ static int pthreadpool_tevent_job_signal(int jobid, } } + ret = pthread_mutex_unlock(&state->pool->glue_mutex); + if (ret != 0) { + return ret; + } + if (tctx == NULL) { abort(); }