Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #243: race in qdr_close_connection_CT #213

Merged

Conversation

jiridanek
Copy link
Contributor

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)

@jiridanek jiridanek changed the title Fix race in closing connection Fixed #243: race in qdr_close_connection_CT Mar 25, 2022
@jiridanek jiridanek marked this pull request as ready for review March 25, 2022 20:51
@@ -305,12 +307,14 @@ int qdr_connection_process(qdr_connection_t *conn)

int event_count = 0;

sys_mutex_lock(conn->work_lock);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not think very hard if this is the correct lock to use. It seemed conveniently placed, so I just extended what it protects by a few lines up. Actually, the closed flag seems to be tightly bound to the list of links with work, so it may make good sense this way, in fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgiusti approved this and the issue has manifested on GHA in controlled environment; therefore, issue is real and fix should be acceptable!

@jiridanek jiridanek force-pushed the jd_2022_03_21_fix_race_closing branch from 0158a83 to faa7546 Compare March 28, 2022 20:13
@jiridanek jiridanek changed the title Fixed #243: race in qdr_close_connection_CT Issue #243: race in qdr_close_connection_CT Apr 4, 2022
```
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)
```
@jiridanek jiridanek force-pushed the jd_2022_03_21_fix_race_closing branch from faa7546 to 395ea3b Compare April 4, 2022 09:26
@jiridanek jiridanek merged commit 3e070de into skupperproject:main Apr 7, 2022
@jiridanek jiridanek deleted the jd_2022_03_21_fix_race_closing branch April 7, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants