8000 Lock before checking `ares_event_thread::isup` by jimmy-park · Pull Request #915 · c-ares/c-ares · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Lock before checking ares_event_thread::isup #915

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 9, 2024
Merged

Lock before checking ares_event_thread::isup #915

merged 1 commit into from
Nov 9, 2024

Conversation

jimmy-park
Copy link
Contributor

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-ar
8000
es/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
26 of 28 checks passed
@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
0