From 9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af Mon Sep 17 00:00:00 2001
From: practicalswift <practicalswift@users.noreply.github.com>
Date: Wed, 4 Apr 2018 14:04:10 +0200
Subject: 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`).
---
 src/test/denialofservice_tests.cpp | 76 +++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 22 deletions(-)

(limited to 'src/test')

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index e5f914ba8a..bebbd6c464 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_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);
-- 
cgit v1.2.3