Issue #243: race in qdr_close_connection_CT

``` WARNING: ThreadSanitizer: data race (pid=3244) Write of size 1 at 0x7b54000c9942 by thread T1: #0 qdr_close_connection_CT src/router_core/connections.c:267 (skrouterd+0x4875d8) #1 qdr_core_close_connection_CT src/router_core/connections.c:283 (skrouterd+0x487766) #2 router_core_thread src/router_core/router_core_thread.c:236 (skrouterd+0x4a382a) #3 _thread_init src/posix/threading.c:172 (skrouterd+0x47ad5d) Previous read of size 1 at 0x7b54000c9942 by main thread: #0 qdr_connection_process src/router_core/connections.c:308 (skrouterd+0x48783c) #1 _do_reconnect src/adaptors/http1/http1_server.c:432 (skrouterd+0x43bfcc) #2 qd_timer_visit src/timer.c:320 (skrouterd+0x4c587f) #3 handle src/server.c:980 (skrouterd+0x4c114e) #4 thread_run src/server.c:1095 (skrouterd+0x4c2f57) #5 qd_server_run src/server.c:1491 (skrouterd+0x4c3acc) #6 main_process router/src/main.c:105 (skrouterd+0x424e5c) #7 main router/src/main.c:359 (skrouterd+0x4242ec) Location is heap block of size 640 at 0x7b54000c9900 allocated by thread T4: #0 posix_memalign <null> (libtsan.so.0+0x32a23) #1 qd_alloc src/alloc_pool.c:396 (skrouterd+0x448d09) #2 new_qdr_connection_t src/router_core/connections.c:44 (skrouterd+0x486d21) #3 qdr_connection_opened src/router_core/connections.c:88 (skrouterd+0x486d21) #4 _setup_client_connection src/adaptors/http1/http1_client.c:316 (skrouterd+0x4342da) #5 _handle_connection_events src/adaptors/http1/http1_client.c:452 (skrouterd+0x4342da) #6 handle_event_with_context src/server.c:780 (skrouterd+0x4c11f9) #7 do_handle_raw_connection_event src/server.c:786 (skrouterd+0x4c11f9) #8 handle src/server.c:1063 (skrouterd+0x4c11f9) #9 thread_run src/server.c:1095 (skrouterd+0x4c2f57) #10 _thread_init src/posix/threading.c:172 (skrouterd+0x47ad5d) ```
skupperproject · Apr 4, 2022 · 395ea3b · 395ea3b
1 parent 716ce32
commit 395ea3b
Showing 1 changed file with 7 additions and 1 deletion.
diff --git a/src/router_core/connections.c b/src/router_core/connections.c
@@ -264,9 +264,11 @@ void qdr_record_link_credit(qdr_core_t *core, qdr_link_t *link)
 
 void qdr_close_connection_CT(qdr_core_t *core, qdr_connection_t  *conn)
 {
+    sys_mutex_lock(conn->work_lock);
     conn->closed = true;
     conn->error  = qdr_error(QD_AMQP_COND_CONNECTION_FORCED, "Connection forced-closed by management request");
     conn->admin_status = QD_CONN_ADMIN_DELETED;
+    sys_mutex_unlock(conn->work_lock);
 
     //Activate the connection, so the I/O threads can finish the job.
     qdr_connection_activate_CT(core, conn);
@@ -305,12 +307,16 @@ int qdr_connection_process(qdr_connection_t *conn)
 
     int event_count = 0;
 
+    sys_mutex_lock(conn->work_lock);
+
     if (conn->closed) {
+        sys_mutex_unlock(conn->work_lock);
+        // Unlock is called before calling into the conn_close_handler.
+        // The handler's pluggable and we don't control which locks it takes (or may take in the future).
         conn->protocol_adaptor->conn_close_handler(conn->protocol_adaptor->user_context, conn, conn->error);
         return 0;
     }
 
-    sys_mutex_lock(conn->work_lock);
     DEQ_MOVE(conn->work_list, work_list);
     for (int priority = 0; priority <= QDR_MAX_PRIORITY; ++ priority) {
         DEQ_MOVE(conn->links_with_work[priority], links_with_work[priority]);