Skip to content

Lock before checking ares_event_thread::isup#915

Merged
bradh352 merged 1 commit intoc-ares:mainfrom
jimmy-park:fix-data-race
Nov 9, 2024
Merged

Lock before checking ares_event_thread::isup#915
bradh352 merged 1 commit intoc-ares:mainfrom
jimmy-park:fix-data-race

Conversation

@jimmy-park
Copy link
Contributor

@jimmy-park jimmy-park commented Nov 9, 2024

I found a missing lock protection inside the event thread loop.

==================
WARNING: ThreadSanitizer: data race (pid=4351)
  Write of size 4 at 0x7b1800000120 by main thread (mutexes: write M0):
    #0 ares_event_thread_destroy_int /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:380:13 (arestest+0x4b83e1) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #1 ares_event_thread_destroy /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:410:3 (arestest+0x4b82e8) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #2 ares_destroy /home/runner/work/c-ares/c-ares/src/lib/ares_destroy.c:107:5 (arestest+0x48c143) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #3 ares::test::MockChannelOptsTest::~MockChannelOptsTest() /home/runner/work/c-ares/c-ares/test/ares-test.cc:871:5 (arestest+0x1614f8) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #4 ares::test::MockEventThreadOptsTest::~MockEventThreadOptsTest() /home/runner/work/c-ares/c-ares/test/ares-test.h:392:3 (arestest+0x3c15b5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #5 ares::test::ServerFailoverOptsMockEventThreadTest::~ServerFailoverOptsMockEventThreadTest() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1427:7 (arestest+0x3e0239) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #6 ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test::~ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1457:1 (arestest+0x3bea45) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #7 ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test::~ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1457:1 (arestest+0x3bea89) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #8 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) <null> (arestest+0x51046e) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #9 main /home/runner/work/c-ares/c-ares/test/ares-test-main.cc:107:12 (arestest+0x140114) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)

  Previous read of size 4 at 0x7b1800000120 by thread T527:
    #0 ares_event_thread /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:359:12 (arestest+0x4b937b) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)

  Location is heap block of size 96 at 0x7b1800000120 allocated by main thread:
    #0 malloc <null> (arestest+0xbd5b1) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #1 ares::test::LibraryTest::amalloc(unsigned long) /home/runner/work/c-ares/c-ares/test/ares-test.cc:387:12 (arestest+0x15ba6c) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #2 ares_malloc /home/runner/work/c-ares/c-ares/src/lib/ares_library_init.c:71:10 (arestest+0x495bd5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #3 ares_malloc_zero /home/runner/work/c-ares/c-ares/src/lib/ares_library_init.c:86:15 (arestest+0x495cc5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #4 ares_event_thread_init /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:480:7 (arestest+0x4b8509) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #5 ares_init_options /home/runner/work/c-ares/c-ares/src/lib/ares_init.c:355:14 (arestest+0x49444c) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #6 ares::test::MockChannelOptsTest::MockChannelOptsTest(int, int, bool, bool, ares_options*, int) /home/runner/work/c-ares/c-ares/test/ares-test.cc:830:3 (arestest+0x160933) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #7 ares::test::MockEventThreadOptsTest::MockEventThreadOptsTest(int, ares_evsys_t, int, bool, ares_options*, int) /home/runner/work/c-ares/c-ares/test/ares-test.h:384:7 (arestest+0x3c1306) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #8 ares::test::ServerFailoverOptsMockEventThreadTest::ServerFailoverOptsMockEventThreadTest() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1432:7 (arestest+0x3e00e5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #9 ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1457:1 (arestest+0x3dff49) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #10 testing::internal::ParameterizedTestFactory<ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test>::CreateTest() /usr/include/gtest/internal/gtest-param-util.h:401:16 (arestest+0x3dfeb5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #11 testing::Test* testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) <null> (arestest+0x510666) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #12 main /home/runner/work/c-ares/c-ares/test/ares-test-main.cc:107:12 (arestest+0x140114) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)

  Mutex M0 (0x7b0c00013410) created at:
    #0 pthread_mutex_init <null> (arestest+0xc04ef) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #1 ares_thread_mutex_create /home/runner/work/c-ares/c-ares/src/lib/util/ares_threads.c:235:7 (arestest+0x4d3c47) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #2 ares_event_thread_init /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:485:14 (arestest+0x4b8529) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #3 ares_init_options /home/runner/work/c-ares/c-ares/src/lib/ares_init.c:355:14 (arestest+0x49444c) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #4 ares::test::MockChannelOptsTest::MockChannelOptsTest(int, int, bool, bool, ares_options*, int) /home/runner/work/c-ares/c-ares/test/ares-test.cc:830:3 (arestest+0x160933) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #5 ares::test::MockEventThreadOptsTest::MockEventThreadOptsTest(int, ares_evsys_t, int, bool, ares_options*, int) /home/runner/work/c-ares/c-ares/test/ares-test.h:384:7 (arestest+0x3c1306) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #6 ares::test::ServerFailoverOptsMockEventThreadTest::ServerFailoverOptsMockEventThreadTest() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1432:7 (arestest+0x3e00e5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #7 ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1457:1 (arestest+0x3dff49) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #8 testing::internal::ParameterizedTestFactory<ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test>::CreateTest() /usr/include/gtest/internal/gtest-param-util.h:401:16 (arestest+0x3dfeb5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #9 testing::Test* testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) <null> (arestest+0x510666) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #10 main /home/runner/work/c-ares/c-ares/test/ares-test-main.cc:107:12 (arestest+0x140114) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)

  Thread T527 (tid=4903, running) created by main thread at:
    #0 pthread_create <null> (arestest+0xbed1d) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #1 ares_thread_create /home/runner/work/c-ares/c-ares/src/lib/util/ares_threads.c:384:7 (arestest+0x4d4268) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #2 ares_event_thread_init /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:539:7 (arestest+0x4b8916) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #3 ares_init_options /home/runner/work/c-ares/c-ares/src/lib/ares_init.c:355:14 (arestest+0x49444c) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #4 ares::test::MockChannelOptsTest::MockChannelOptsTest(int, int, bool, bool, ares_options*, int) /home/runner/work/c-ares/c-ares/test/ares-test.cc:830:3 (arestest+0x160933) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #5 ares::test::MockEventThreadOptsTest::MockEventThreadOptsTest(int, ares_evsys_t, int, bool, ares_options*, int) /home/runner/work/c-ares/c-ares/test/ares-test.h:384:7 (arestest+0x3c1306) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #6 ares::test::ServerFailoverOptsMockEventThreadTest::ServerFailoverOptsMockEventThreadTest() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1432:7 (arestest+0x3e00e5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #7 ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test() /home/runner/work/c-ares/c-ares/test/ares-test-mock-et.cc:1457:1 (arestest+0x3dff49) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #8 testing::internal::ParameterizedTestFactory<ares::test::ServerFailoverOptsMockEventThreadTest_ServerFailoverOpts_Test>::CreateTest() /usr/include/gtest/internal/gtest-param-util.h:401:16 (arestest+0x3dfeb5) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #9 testing::Test* testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test*>(testing::internal::TestFactoryBase*, testing::Test* (testing::internal::TestFactoryBase::*)(), char const*) <null> (arestest+0x510666) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)
    #10 main /home/runner/work/c-ares/c-ares/test/ares-test-main.cc:107:12 (arestest+0x140114) (BuildId: 43d258f5ba006c3f84530170484b8627def73923)

SUMMARY: ThreadSanitizer: data race /home/runner/work/c-ares/c-ares/src/lib/event/ares_event_thread.c:380:13 in ares_event_thread_destroy_int
==================

@bradh352
Copy link
Member

bradh352 commented Nov 9, 2024

In theory, reads and writes to a boolean should always be atomic so a mutex shouldn't be needed. Plus in this case there's not much harm if ares_process_fds() gets called during the shutdown process, it just wastes a few cpu cycles. So not actually a real issue, but ... if its reported by TSAN then we should fix it so someone else using c-ares doesn't get the noise.

@bradh352 bradh352 merged commit bd5561b into c-ares:main Nov 9, 2024
@jimmy-park jimmy-park deleted the fix-data-race branch November 9, 2024 12:45
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
f89f3ca 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
15883a9 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
ac352d7 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
81b1f7d 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
bac27ee 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
10b7bc7 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
1f85d72 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
7b3ae34 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
bradh352 pushed a commit that referenced this pull request Nov 9, 2024
@jimmy-park @bradh352
Silence TSAN, lock before checking ares_event_thread::isup (#915)
ee51940 
TSAN is warning about a thread concurrency issue that doesn't actually matter if the operation isn't atomic as its an optimization in this code path to skip timeout processing if we're shutting down due to ares_destroy().

Fix By: Jiwoo Park (@jimmy-park)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jimmy-park @bradh352