tests: Make test_bitcoin pass under ThreadSanitzer (clang). Fix lock-order-inversion (potential deadlock). #12882

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:lock-order-inversion-in-DoS_tests changing 2 files +55 −23
  1. practicalswift commented at 12:05 PM on April 4, 2018: contributor

    Fix lock-order-inversion (potential deadlock) in DoS_tests. Reported by Clang's TSAN.

    Makes src/test/test_bitcoin pass also when compiled with TreadSanitizer (./configure --with-sanitizers=thread with clang).

  2. fanquake added the label Tests on Apr 4, 2018
  3. practicalswift renamed this:
    tests: Fix lock-order-inversion (potential deadlock) in DoS_tests. Reported by TSAN.
    tests: Make test_bitcoin pass under ThreadSanitzer. Fix lock-order-inversion (potential deadlock) in DoS_tests.
    on Apr 4, 2018
  4. practicalswift renamed this:
    tests: Make test_bitcoin pass under ThreadSanitzer. Fix lock-order-inversion (potential deadlock) in DoS_tests.
    tests: Make test_bitcoin pass under ThreadSanitzer. Fix lock-order-inversion (potential deadlock).
    on Apr 4, 2018
  5. practicalswift renamed this:
    tests: Make test_bitcoin pass under ThreadSanitzer. Fix lock-order-inversion (potential deadlock).
    tests: Make test_bitcoin pass under ThreadSanitzer (clang). Fix lock-order-inversion (potential deadlock).
    on Apr 4, 2018
  6. fanquake commented at 7:59 AM on April 16, 2018: member

    @practicalswift Can you include some more information about recreating this? I've done a naive:

    ./autogen.sh && ./configure --with-sanitizers=thread && make check -j6

    (macOS 10.13.4 with XCode 9.3) and am having trouble recreating the issue.

  7. practicalswift commented at 9:04 AM on April 16, 2018: contributor

    @fanquake I just verified using clang version 7.0.0 (trunk) under Linux using the following commands:

    $ git clone https://github.com/bitcoin/bitcoin
$ cd bitcoin/
$ ./autogen.sh
$ CC=clang-7 CXX=clang++-7 ./configure --disable-asm --enable-debug --with-sanitizers=thread --disable-wallet
$ make check
…
SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) (/root/repro/bitcoin/src/test/test_bitcoin+0xc5897) in pthread_mutex_lock
  8. practicalswift commented at 7:32 PM on April 16, 2018: contributor

    @sipa As a fellow friend of the sanitizers (#9743, #9512), would you mind reviewing this one? :-)

  9. MarcoFalke commented at 8:02 PM on April 16, 2018: member

    It could help reviewers if you explained why it is a deadlock in the OP and then also describe why your patch fixes it.

  10. practicalswift commented at 8:31 PM on April 16, 2018: contributor

    @MarcoFalke Ah, sorry: the reason for the potential deadlock warning (lock-order-inversion) is simply that the lock order between cs_main and foo->cs_sendProcessing differed between tests prior to this commit.

    After this commit the following lock order is used:

    1. LOCK(cs_main);
2. LOCK(foo->cs_sendProcessing);

    Prior to this commit the following lock order was also used:

    1. LOCK(foo->cs_sendProcessing);
2. LOCK(cs_main);

    This is part of the -fsanitize=thread output:

    WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=20357)
  Cycle in lock order graph: M6554537 (0x55cf759864f8) => M453033967624932984 (0x000000000000) => M6554537

  Mutex M453033967624932984 acquired here while holding mutex M6554537 in main thread:
  …
    [#8](/bitcoin-bitcoin/8/) DoS_tests::outbound_slow_chain_eviction::test_method() /…/bitcoin/src/test/DoS_tests.cpp:74:5 (test_bitcoin+0x376064)
    [#9](/bitcoin-bitcoin/9/) DoS_tests::outbound_slow_chain_eviction_invoker() /…/bitcoin/src/test/DoS_tests.cpp:55:1 (test_bitcoin+0x375782)
  …
  Mutex M6554537 acquired here while holding mutex M453033967624932984 in main thread:
  …
    [#8](/bitcoin-bitcoin/8/) PeerLogicValidation::InitializeNode(CNode*) /…/bitcoin/src/net_processing.cpp:581:9 (test_bitcoin+0x8b955c)
    [#9](/bitcoin-bitcoin/9/) DoS_tests::DoS_banning::test_method() /…/bitcoin/src/test/DoS_tests.cpp:201:16 (test_bitcoin+0x379a18)
    [#10](/bitcoin-bitcoin/10/) DoS_tests::DoS_banning_invoker() /…/bitcoin/src/test/DoS_tests.cpp:178:1 (test_bitcoin+0x378e42)
  11. fanquake commented at 5:08 AM on April 17, 2018: member

    Thanks @practicalswift. I've recreated the potential deadlock on Ubuntu using clang 6.0.0-1ubuntu2 (tags/RELEASE_600/final):

    Running tests: DoS_tests from test/DoS_tests.cpp
Running 6 test cases...
Entering test module "Bitcoin Test Suite"
test/DoS_tests.cpp(45): Entering test suite "DoS_tests"
test/DoS_tests.cpp(55): Entering test case "outbound_slow_chain_eviction"
test/DoS_tests.cpp(55): Leaving test case "outbound_slow_chain_eviction"; testing time: 320057us
test/DoS_tests.cpp(109): Entering test case "stale_tip_peer_management"
test/DoS_tests.cpp(109): Leaving test case "stale_tip_peer_management"; testing time: 254718us
test/DoS_tests.cpp(178): Entering test case "DoS_banning"
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=14733)
  Cycle in lock order graph: M131611 (0x556f72060500) => M131679 (0x7ffe31689738) => M131611

  Mutex M131679 acquired here while holding mutex M131611 in main thread:
    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    [#1](/bitcoin-bitcoin/1/) __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:748 (test_bitcoin+0x158dc3)
    [#2](/bitcoin-bitcoin/2/) __gthread_recursive_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:810 (test_bitcoin+0x158d45)
    [#3](/bitcoin-bitcoin/3/) std::recursive_mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/mutex:107 (test_bitcoin+0x15e415)
    [#4](/bitcoin-bitcoin/4/) AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    [#5](/bitcoin-bitcoin/5/) std::unique_lock<CCriticalSection>::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/std_mutex.h:267 (test_bitcoin+0x15e2fe)
    [#6](/bitcoin-bitcoin/6/) CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    [#7](/bitcoin-bitcoin/7/) CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    [#8](/bitcoin-bitcoin/8/) DoS_tests::outbound_slow_chain_eviction::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:74 (test_bitcoin+0x3a90a3)
    [#9](/bitcoin-bitcoin/9/) DoS_tests::outbound_slow_chain_eviction_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:55 (test_bitcoin+0x3a863e)
    [#10](/bitcoin-bitcoin/10/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:118 (test_bitcoin+0x1dde4a)
    [#11](/bitcoin-bitcoin/11/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) ??:? (libboost_unit_test_framework.so.1.65.1+0x4b2cd)
    [#12](/bitcoin-bitcoin/12/) __libc_start_main ??:? (libc.so.6+0x21b96)

    Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message

  Mutex M131611 acquired here while holding mutex M131679 in main thread:
    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    [#1](/bitcoin-bitcoin/1/) __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:748 (test_bitcoin+0x158dc3)
    [#2](/bitcoin-bitcoin/2/) __gthread_recursive_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:810 (test_bitcoin+0x158d45)
    [#3](/bitcoin-bitcoin/3/) std::recursive_mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/mutex:107 (test_bitcoin+0x15e415)
    [#4](/bitcoin-bitcoin/4/) AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    [#5](/bitcoin-bitcoin/5/) std::unique_lock<CCriticalSection>::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/std_mutex.h:267 (test_bitcoin+0x15e2fe)
    [#6](/bitcoin-bitcoin/6/) CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    [#7](/bitcoin-bitcoin/7/) CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    [#8](/bitcoin-bitcoin/8/) PeerLogicValidation::InitializeNode(CNode*) /home/ubuntu/bitcoin/src/net_processing.cpp:583 (test_bitcoin+0x92b76c)
    [#9](/bitcoin-bitcoin/9/) DoS_tests::DoS_banning::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:201 (test_bitcoin+0x3acf87)
    [#10](/bitcoin-bitcoin/10/) DoS_tests::DoS_banning_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:178 (test_bitcoin+0x3ac22e)
    [#11](/bitcoin-bitcoin/11/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:118 (test_bitcoin+0x1dde4a)
    [#12](/bitcoin-bitcoin/12/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) ??:? (libboost_unit_test_framework.so.1.65.1+0x4b2cd)
    [#13](/bitcoin-bitcoin/13/) __libc_start_main ??:? (libc.so.6+0x21b96)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) ??:? in pthread_mutex_lock
==================
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=14733)
  Cycle in lock order graph: M131611 (0x556f72060500) => M131679 (0x7ffe31689738) => M131611

  Mutex M131679 acquired here while holding mutex M131611 in main thread:
    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    [#1](/bitcoin-bitcoin/1/) __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:748 (test_bitcoin+0x158dc3)
    [#2](/bitcoin-bitcoin/2/) __gthread_recursive_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:810 (test_bitcoin+0x158d45)
    [#3](/bitcoin-bitcoin/3/) std::recursive_mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/mutex:107 (test_bitcoin+0x15e415)
    [#4](/bitcoin-bitcoin/4/) AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    [#5](/bitcoin-bitcoin/5/) std::unique_lock<CCriticalSection>::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/std_mutex.h:267 (test_bitcoin+0x15e2fe)
    [#6](/bitcoin-bitcoin/6/) CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    [#7](/bitcoin-bitcoin/7/) CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    [#8](/bitcoin-bitcoin/8/) DoS_tests::outbound_slow_chain_eviction::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:74 (test_bitcoin+0x3a90a3)
    [#9](/bitcoin-bitcoin/9/) DoS_tests::outbound_slow_chain_eviction_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:55 (test_bitcoin+0x3a863e)
    [#10](/bitcoin-bitcoin/10/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:118 (test_bitcoin+0x1dde4a)
    [#11](/bitcoin-bitcoin/11/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) ??:? (libboost_unit_test_framework.so.1.65.1+0x4b2cd)
    [#12](/bitcoin-bitcoin/12/) __libc_start_main ??:? (libc.so.6+0x21b96)

    Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message

  Mutex M131611 acquired here while holding mutex M131679 in main thread:
    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    [#1](/bitcoin-bitcoin/1/) __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:748 (test_bitcoin+0x158dc3)
    [#2](/bitcoin-bitcoin/2/) __gthread_recursive_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/x86_64-linux-gnu/c++/8.0.1/bits/gthr-default.h:810 (test_bitcoin+0x158d45)
    [#3](/bitcoin-bitcoin/3/) std::recursive_mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/mutex:107 (test_bitcoin+0x15e415)
    [#4](/bitcoin-bitcoin/4/) AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    [#5](/bitcoin-bitcoin/5/) std::unique_lock<CCriticalSection>::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/bits/std_mutex.h:267 (test_bitcoin+0x15e2fe)
    [#6](/bitcoin-bitcoin/6/) CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    [#7](/bitcoin-bitcoin/7/) CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    [#8](/bitcoin-bitcoin/8/) PeerLogicValidation::InitializeNode(CNode*) /home/ubuntu/bitcoin/src/net_processing.cpp:583 (test_bitcoin+0x92b76c)
    [#9](/bitcoin-bitcoin/9/) DoS_tests::DoS_banning::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:201 (test_bitcoin+0x3acf87)
    [#10](/bitcoin-bitcoin/10/) DoS_tests::DoS_banning_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:178 (test_bitcoin+0x3ac22e)
    [#11](/bitcoin-bitcoin/11/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:118 (test_bitcoin+0x1dde4a)
    [#12](/bitcoin-bitcoin/12/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) ??:? (libboost_unit_test_framework.so.1.65.1+0x4b2cd)
    [#13](/bitcoin-bitcoin/13/) __libc_start_main ??:? (libc.so.6+0x21b96)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) ??:? in pthread_mutex_lock
==================
test/DoS_tests.cpp(178): Leaving test case "DoS_banning"; testing time: 148936us
test/DoS_tests.cpp(224): Entering test case "DoS_banscore"
test/DoS_tests.cpp(224): Leaving test case "DoS_banscore"; testing time: 117896us
test/DoS_tests.cpp(261): Entering test case "DoS_bantime"
test/DoS_tests.cpp(261): Leaving test case "DoS_bantime"; testing time: 109843us
test/DoS_tests.cpp(304): Entering test case "DoS_mapOrphans"
test/DoS_tests.cpp(304): Leaving test case "DoS_mapOrphans"; testing time: 2757430us
test/DoS_tests.cpp(45): Leaving test suite "DoS_tests"; testing time: 3709153us
Leaving test module "Bitcoin Test Suite"; testing time: 3709218us

*** No errors detected
ThreadSanitizer: reported 2 warnings

    Can confirm that this patch make the warning disappear.

  12. MarcoFalke commented at 1:23 PM on April 17, 2018: member

    So what exactly needs cs_sendProcessing and why is it not scoped on that?

  13. practicalswift commented at 1:58 PM on April 17, 2018: contributor

    @MarcoFalke SendMessages(…) requires holding cs_sendProcessing, right?

  14. MarcoFalke commented at 2:10 PM on April 17, 2018: member

    Can't vouch for that, but it seems so.

  15. MarcoFalke commented at 7:57 PM on April 18, 2018: member

    @practicalswift Mind to update according to my suggestion?

  16. practicalswift commented at 10:27 PM on April 18, 2018: contributor

    @MarcoFalke Do you mean with more narrowly scoped locks? Something like this?

    diff --git a/src/net_processing.h b/src/net_processing.h
index 195d0d2..6802887 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -72,7 +72,7 @@ public:
     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   interrupt       Interrupt condition for processing threads
     * [@return](/bitcoin-bitcoin/contributor/return/)                      True if there is more work to be done
     */
-    bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override;
+    bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);

     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
     void ConsiderEviction(CNode *pto, int64_t time_in_seconds);
diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp
index abc31e6..3d1ac7a 100644
--- a/src/test/DoS_tests.cpp
+++ b/src/test/DoS_tests.cpp
@@ -66,25 +66,40 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     dummyNode1.fSuccessfullyConnected = true;

     // This test requires that we have a chain with non-zero work.
-    LOCK(cs_main);
-    BOOST_CHECK(chainActive.Tip() != nullptr);
-    BOOST_CHECK(chainActive.Tip()->nChainWork > 0);
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(chainActive.Tip() != nullptr);
+        BOOST_CHECK(chainActive.Tip()->nChainWork > 0);
+    }

     // Test starts here
-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
-    LOCK(dummyNode1.cs_vSend);
-    BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
-    dummyNode1.vSendMsg.clear();
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
+    }
+    {
+        LOCK2(cs_main, dummyNode1.cs_vSend);
+        BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
+        dummyNode1.vSendMsg.clear();
+    }

     int64_t nStartTime = GetTime();
     // Wait 21 minutes
     SetMockTime(nStartTime+21*60);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
-    BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
+    }
+    {
+        LOCK2(cs_main, dummyNode1.cs_vSend);
+        BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
+    }
     // Wait 3 more minutes
     SetMockTime(nStartTime+24*60);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
+    }
     BOOST_CHECK(dummyNode1.fDisconnect == true);
     SetMockTime(0);

@@ -190,8 +205,10 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 100); // Should get banned
     }
-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr1));
     BOOST_CHECK(!connman->IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned

@@ -205,15 +222,20 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK(cs_main);
         Misbehaving(dummyNode2.GetId(), 50);
     }
-    LOCK(dummyNode2.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode2.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    }
     BOOST_CHECK(!connman->IsBanned(addr2)); // 2 not banned yet...
     BOOST_CHECK(connman->IsBanned(addr1));  // ... but 1 still should be
     {
         LOCK(cs_main);
         Misbehaving(dummyNode2.GetId(), 50);
     }
-    peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode2.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr2));

     bool dummy;
@@ -237,20 +259,28 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 100);
     }
-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(!connman->IsBanned(addr1));
     {
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 10);
     }
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(!connman->IsBanned(addr1));
     {
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 1);
     }
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr1));
     gArgs.ForceSetArg("-banscore", std::to_string(DEFAULT_BANSCORE_THRESHOLD));

@@ -277,8 +307,10 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
         LOCK(cs_main);
         Misbehaving(dummyNode.GetId(), 100);
     }
-    LOCK(dummyNode.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr));

     SetMockTime(nStartTime+60*60);
  17. MarcoFalke commented at 2:56 PM on April 19, 2018: member

    @practicalswift Looks very verbose, but fine to me. Shouldn't hurt to document the locking assumtions in tests?

  18. practicalswift commented at 3:04 PM on April 19, 2018: contributor

    @MarcoFalke Do you have an example on how to document the locking assumption in the tests?

    That is beyond the suggested documentation in form of the annotation:

    bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override 
    EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
  19. MarcoFalke commented at 3:05 PM on April 19, 2018: member

    I think your suggested diff is fine

  20. practicalswift force-pushed on Apr 19, 2018
  21. practicalswift commented at 3:15 PM on April 19, 2018: contributor

    @MarcoFalke Applied. Please review :-)

  22. in src/test/DoS_tests.cpp:209 in 8043568a24 outdated 
     205 | @@ -190,8 +206,10 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
 206 |          LOCK(cs_main);
 207 |          Misbehaving(dummyNode1.GetId(), 100); // Should get banned
 208 |      }
 209 | -    LOCK(dummyNode1.cs_sendProcessing);
 210 | -    peerLogic->SendMessages(&dummyNode1, interruptDummy);
 211 | +    {
 212 | +        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
    MarcoFalke commented at 4:21 PM on April 19, 2018:

    nit: Seems you already locked cs_main in the first line of the test case, so a LOCK(dummyNode1.cs_sendProcessing); should suffice

    practicalswift commented at 8:00 AM on April 20, 2018:

    Fixed!

  23. practicalswift force-pushed on Apr 19, 2018
  24. practicalswift commented at 4:41 PM on April 19, 2018: contributor

    @MarcoFalke Thanks! Please review updated version :-)

  25. MarcoFalke commented at 9:04 PM on April 19, 2018: member

    utACK 640523f77882c655ade8373eea1062691bf72b98

  26. practicalswift commented at 1:27 PM on May 14, 2018: contributor

    @promag @laanwj @fanquake Would you mind reviewing? :-)

  27. DrahtBot added the label Needs rebase on Jun 12, 2018
  28. DrahtBot commented at 4:51 PM on June 12, 2018: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  29. tests: Fix lock-order-inversion (potential deadlock) in DoS_tests. Reported by TSAN.
    Makes `src/test/test_bitcoin --run_test=DoS_tests` pass also when
compiled with TreadSanitizer (`./configure --with-sanitizers=thread`).
    9fdf05d70c
  30. practicalswift force-pushed on Jun 12, 2018
  31. practicalswift commented at 7:46 PM on June 12, 2018: contributor

    Rebased! Please re-review :-)

  32. DrahtBot removed the label Needs rebase on Jun 12, 2018
  33. sipa commented at 2:32 AM on June 27, 2018: member

    utACK 9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af

  34. MarcoFalke commented at 10:30 AM on June 27, 2018: member

    utACK 9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af

  35. MarcoFalke merged this on Jun 27, 2018
  36. MarcoFalke closed this on Jun 27, 2018
  37. MarcoFalke referenced this in commit d96bdd7830 on Jun 27, 2018
  38. MarcoFalke commented at 1:52 PM on July 14, 2018: member

    Can we enable this in travis, so it wouldn't happen again, please?

  39. PastaPastaPasta referenced this in commit cc662bdd15 on Jul 7, 2020
  40. PastaPastaPasta referenced this in commit 53b36a6838 on Jul 7, 2020
  41. PastaPastaPasta referenced this in commit b1a1954e5b on Jul 8, 2020
  42. practicalswift deleted the branch on Apr 10, 2021
  43. gades referenced this in commit 285f1d2a1b on Feb 10, 2022
  44. DrahtBot locked this on Aug 18, 2022
Contributors
practicalswiftfanquakeMarcoFalkeDrahtBotsipa
Labels
Tests
Linked (view graph)
#13129 Meta-issue: Add Clang thread safety analysis annotations
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me