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 <null> <null> (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 <null> (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 <null> (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 <noel.power@suse.com>
Reviewed-by: Stefan Metzmacher <metze@samba.org>

diff --git a/lib/pthreadpool/pthreadpool_tevent.c b/lib/pthreadpool/pthreadpool_tevent.c
index 515dcc0fd7bc9f7d121f378de399fc23a27e5eb8..19cf2dfbc15239a4bec25ee51983ecaa2543de47 100644 (file)
--- 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();
        }