From 9fdf05d70cac4a62d1aeeb4299e2c3a9a866f8af Mon Sep 17 00:00:00 2001 From: practicalswift Date: Wed, 4 Apr 2018 14:04:10 +0200 Subject: [PATCH] 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/net_processing.h | 2 +- src/test/denialofservice_tests.cpp | 76 +++++++++++++++++++++--------- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/src/net_processing.h b/src/net_processing.h index b0b905d92..3bdb4785a 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -77,7 +77,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& interrupt) override; + bool SendMessages(CNode* pto, std::atomic& 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/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index e5f914ba8..bebbd6c46 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);