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

practicalswift · 2018-04-04T12:05:54Z

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).

fanquake · 2018-04-16T07:59:05Z

@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.

practicalswift · 2018-04-16T09:04:43Z

@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

practicalswift · 2018-04-16T19:32:08Z

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

maflcko · 2018-04-16T20:02:03Z

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

practicalswift · 2018-04-16T20:31:57Z

@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 DoS_tests::outbound_slow_chain_eviction::test_method() /…/bitcoin/src/test/DoS_tests.cpp:74:5 (test_bitcoin+0x376064)
    #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 PeerLogicValidation::InitializeNode(CNode*) /…/bitcoin/src/net_processing.cpp:581:9 (test_bitcoin+0x8b955c)
    #9 DoS_tests::DoS_banning::test_method() /…/bitcoin/src/test/DoS_tests.cpp:201:16 (test_bitcoin+0x379a18)
    #10 DoS_tests::DoS_banning_invoker() /…/bitcoin/src/test/DoS_tests.cpp:178:1 (test_bitcoin+0x378e42)

fanquake · 2018-04-17T05:08:31Z

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 pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    #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 __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 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 AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    #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 CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    #7 CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    #8 DoS_tests::outbound_slow_chain_eviction::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:74 (test_bitcoin+0x3a90a3)
    #9 DoS_tests::outbound_slow_chain_eviction_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:55 (test_bitcoin+0x3a863e)
    #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 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 __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 pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    #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 __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 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 AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    #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 CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    #7 CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    #8 PeerLogicValidation::InitializeNode(CNode*) /home/ubuntu/bitcoin/src/net_processing.cpp:583 (test_bitcoin+0x92b76c)
    #9 DoS_tests::DoS_banning::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:201 (test_bitcoin+0x3acf87)
    #10 DoS_tests::DoS_banning_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:178 (test_bitcoin+0x3ac22e)
    #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 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 __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 pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    #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 __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 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 AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    #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 CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    #7 CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    #8 DoS_tests::outbound_slow_chain_eviction::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:74 (test_bitcoin+0x3a90a3)
    #9 DoS_tests::outbound_slow_chain_eviction_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:55 (test_bitcoin+0x3a863e)
    #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 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 __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 pthread_mutex_lock ??:? (test_bitcoin+0xcb457)
    #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 __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 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 AnnotatedMixin<std::recursive_mutex>::lock() /home/ubuntu/bitcoin/src/./sync.h:59 (test_bitcoin+0x15e395)
    #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 CCriticalBlock::Enter(char const*, char const*, int) /home/ubuntu/bitcoin/src/./sync.h:128 (test_bitcoin+0x15de07)
    #7 CCriticalBlock /home/ubuntu/bitcoin/src/./sync.h:149 (test_bitcoin+0x1597c7)
    #8 PeerLogicValidation::InitializeNode(CNode*) /home/ubuntu/bitcoin/src/net_processing.cpp:583 (test_bitcoin+0x92b76c)
    #9 DoS_tests::DoS_banning::test_method() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:201 (test_bitcoin+0x3acf87)
    #10 DoS_tests::DoS_banning_invoker() /home/ubuntu/bitcoin/src/test/DoS_tests.cpp:178 (test_bitcoin+0x3ac22e)
    #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 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 __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.

maflcko · 2018-04-17T13:23:48Z

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

practicalswift · 2018-04-17T13:58:01Z

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

maflcko · 2018-04-17T14:10:04Z

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

maflcko · 2018-04-18T19:57:36Z

@practicalswift Mind to update according to my suggestion?

practicalswift · 2018-04-18T22:27:02Z

@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[in]   interrupt       Interrupt condition for processing threads
     * @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);

maflcko · 2018-04-19T14:56:48Z

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

practicalswift · 2018-04-19T15:04:35Z

@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);

maflcko · 2018-04-19T15:05:55Z

I think your suggested diff is fine

practicalswift · 2018-04-19T15:15:46Z

@MarcoFalke Applied. Please review :-)

maflcko · 2018-04-19T16:21:58Z

src/test/DoS_tests.cpp

-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);


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

practicalswift · 2018-04-19T16:41:37Z

@MarcoFalke Thanks! Please review updated version :-)

maflcko · 2018-04-19T21:04:32Z

utACK 640523f77882c655ade8373eea1062691bf72b98

practicalswift · 2018-05-14T13:27:16Z

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

DrahtBot · 2018-06-12T16:51:50Z

Needs rebase

…ported by TSAN. Makes `src/test/test_bitcoin --run_test=DoS_tests` pass also when compiled with TreadSanitizer (`./configure --with-sanitizers=thread`).

practicalswift · 2018-06-12T19:46:13Z

Rebased! Please re-review :-)

sipa · 2018-06-27T02:32:20Z

utACK 9fdf05d

maflcko · 2018-06-27T10:30:23Z

utACK 9fdf05d

…ng). Fix lock-order-inversion (potential deadlock). 9fdf05d tests: Fix lock-order-inversion (potential deadlock) in DoS_tests. Reported by TSAN. (practicalswift) Pull request description: 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`). Tree-SHA512: 41403bb7b6e26bdf1b830b5699e27c637d522bae1799d2a19ed4b68b21b2555438b42170d8b1189613beb32a69b76a65175d29a83f5f4e493896c3d0d94ae26d

maflcko · 2018-07-14T13:52:04Z

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

…er (clang). Fix lock-order-inversion (potential deadlock). 9fdf05d tests: Fix lock-order-inversion (potential deadlock) in DoS_tests. Reported by TSAN. (practicalswift) Pull request description: 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`). Tree-SHA512: 41403bb7b6e26bdf1b830b5699e27c637d522bae1799d2a19ed4b68b21b2555438b42170d8b1189613beb32a69b76a65175d29a83f5f4e493896c3d0d94ae26d

fanquake added the Tests label Apr 4, 2018

practicalswift changed the title ~~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. Apr 4, 2018

practicalswift changed the title ~~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). Apr 4, 2018

practicalswift changed the title ~~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). Apr 4, 2018

practicalswift force-pushed the lock-order-inversion-in-DoS_tests branch from 8124d08 to 8043568 Compare April 19, 2018 15:14

maflcko reviewed Apr 19, 2018

View reviewed changes

practicalswift force-pushed the lock-order-inversion-in-DoS_tests branch from 8043568 to 640523f Compare April 19, 2018 16:32

practicalswift mentioned this pull request Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

30 tasks

DrahtBot added the Needs rebase label Jun 12, 2018

tests: Fix lock-order-inversion (potential deadlock) in DoS_tests. Re…

9fdf05d

…ported by TSAN. Makes `src/test/test_bitcoin --run_test=DoS_tests` pass also when compiled with TreadSanitizer (`./configure --with-sanitizers=thread`).

practicalswift force-pushed the lock-order-inversion-in-DoS_tests branch from 640523f to 9fdf05d Compare June 12, 2018 19:45

DrahtBot removed the Needs rebase label Jun 12, 2018

maflcko merged commit 9fdf05d into bitcoin:master Jun 27, 2018

Bushstar mentioned this pull request Jun 28, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#350

Merged

practicalswift deleted the lock-order-inversion-in-DoS_tests branch April 10, 2021 19:34

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

practicalswift commented Apr 4, 2018 •

edited

Loading

fanquake commented Apr 16, 2018

practicalswift commented Apr 16, 2018 •

edited

Loading

practicalswift commented Apr 16, 2018

maflcko commented Apr 16, 2018

practicalswift commented Apr 16, 2018 •

edited

Loading

fanquake commented Apr 17, 2018

maflcko commented Apr 17, 2018

practicalswift commented Apr 17, 2018 •

edited

Loading

maflcko commented Apr 17, 2018

maflcko commented Apr 18, 2018

practicalswift commented Apr 18, 2018 •

edited

Loading

maflcko commented Apr 19, 2018

practicalswift commented Apr 19, 2018 •

edited

Loading

maflcko commented Apr 19, 2018

practicalswift commented Apr 19, 2018

maflcko Apr 19, 2018

practicalswift Apr 20, 2018

practicalswift commented Apr 19, 2018

maflcko commented Apr 19, 2018

practicalswift commented May 14, 2018

DrahtBot commented Jun 12, 2018

practicalswift commented Jun 12, 2018

sipa commented Jun 27, 2018

maflcko commented Jun 27, 2018

maflcko commented Jul 14, 2018

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

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

Conversation

practicalswift commented Apr 4, 2018 • edited Loading

fanquake commented Apr 16, 2018

practicalswift commented Apr 16, 2018 • edited Loading

practicalswift commented Apr 16, 2018

maflcko commented Apr 16, 2018

practicalswift commented Apr 16, 2018 • edited Loading

fanquake commented Apr 17, 2018

maflcko commented Apr 17, 2018

practicalswift commented Apr 17, 2018 • edited Loading

maflcko commented Apr 17, 2018

maflcko commented Apr 18, 2018

practicalswift commented Apr 18, 2018 • edited Loading

maflcko commented Apr 19, 2018

practicalswift commented Apr 19, 2018 • edited Loading

maflcko commented Apr 19, 2018

practicalswift commented Apr 19, 2018

maflcko Apr 19, 2018

Choose a reason for hiding this comment

practicalswift Apr 20, 2018

Choose a reason for hiding this comment

practicalswift commented Apr 19, 2018

maflcko commented Apr 19, 2018

practicalswift commented May 14, 2018

DrahtBot commented Jun 12, 2018

practicalswift commented Jun 12, 2018

sipa commented Jun 27, 2018

maflcko commented Jun 27, 2018

maflcko commented Jul 14, 2018

practicalswift commented Apr 4, 2018 •

edited

Loading

practicalswift commented Apr 16, 2018 •

edited

Loading

practicalswift commented Apr 16, 2018 •

edited

Loading

practicalswift commented Apr 17, 2018 •

edited

Loading

practicalswift commented Apr 18, 2018 •

edited

Loading

practicalswift commented Apr 19, 2018 •

edited

Loading