From 395ea3bff78b2ed1dd371bd8bebbd37facb57f16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Sat, 19 Mar 2022 18:55:14 +0100 Subject: [PATCH] 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 (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) ``` --- src/router_core/connections.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/router_core/connections.c b/src/router_core/connections.c index e3fc13a6a..38eedc288 100644 --- 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]);