Skip to content

Commit 9fdf05d

Browse files
practicalswiftpracticalswift
committed
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`).
1 parent a607d23 commit 9fdf05d

File tree

2 files changed

+55
-23
lines changed

2 files changed

+55
-23
lines changed

‎src/net_processing.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
7777
* @param[in] interrupt Interrupt condition for processing threads
7878
* @return True if there is more work to be done
7979
*/
80-
bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override;
80+
bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
8181

8282
/** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
8383
void ConsiderEviction(CNode *pto, int64_t time_in_seconds);

‎src/test/denialofservice_tests.cpp‎

Lines changed: 54 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -66,25 +66,40 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
6666
dummyNode1.fSuccessfullyConnected = true;
6767

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

7375
// Test starts here
74-
LOCK(dummyNode1.cs_sendProcessing);
75-
peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
76-
LOCK(dummyNode1.cs_vSend);
77-
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
78-
dummyNode1.vSendMsg.clear();
76+
{
77+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
78+
peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
79+
}
80+
{
81+
LOCK2(cs_main, dummyNode1.cs_vSend);
82+
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
83+
dummyNode1.vSendMsg.clear();
84+
}
7985

8086
int64_t nStartTime = GetTime();
8187
// Wait 21 minutes
8288
SetMockTime(nStartTime+21*60);
83-
peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
84-
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
89+
{
90+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
91+
peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
92+
}
93+
{
94+
LOCK2(cs_main, dummyNode1.cs_vSend);
95+
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
96+
}
8597
// Wait 3 more minutes
8698
SetMockTime(nStartTime+24*60);
87-
peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
99+
{
100+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
101+
peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
102+
}
88103
BOOST_CHECK(dummyNode1.fDisconnect == true);
89104
SetMockTime(0);
90105

@@ -190,8 +205,10 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
190205
LOCK(cs_main);
191206
Misbehaving(dummyNode1.GetId(), 100); // Should get banned
192207
}
193-
LOCK(dummyNode1.cs_sendProcessing);
194-
peerLogic->SendMessages(&dummyNode1, interruptDummy);
208+
{
209+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
210+
peerLogic->SendMessages(&dummyNode1, interruptDummy);
211+
}
195212
BOOST_CHECK(connman->IsBanned(addr1));
196213
BOOST_CHECK(!connman->IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned
197214

@@ -205,15 +222,20 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
205222
LOCK(cs_main);
206223
Misbehaving(dummyNode2.GetId(), 50);
207224
}
208-
LOCK(dummyNode2.cs_sendProcessing);
209-
peerLogic->SendMessages(&dummyNode2, interruptDummy);
225+
{
226+
LOCK2(cs_main, dummyNode2.cs_sendProcessing);
227+
peerLogic->SendMessages(&dummyNode2, interruptDummy);
228+
}
210229
BOOST_CHECK(!connman->IsBanned(addr2)); // 2 not banned yet...
211230
BOOST_CHECK(connman->IsBanned(addr1)); // ... but 1 still should be
212231
{
213232
LOCK(cs_main);
214233
Misbehaving(dummyNode2.GetId(), 50);
215234
}
216-
peerLogic->SendMessages(&dummyNode2, interruptDummy);
235+
{
236+
LOCK2(cs_main, dummyNode2.cs_sendProcessing);
237+
peerLogic->SendMessages(&dummyNode2, interruptDummy);
238+
}
217239
BOOST_CHECK(connman->IsBanned(addr2));
218240

219241
bool dummy;
@@ -237,20 +259,28 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
237259
LOCK(cs_main);
238260
Misbehaving(dummyNode1.GetId(), 100);
239261
}
240-
LOCK(dummyNode1.cs_sendProcessing);
241-
peerLogic->SendMessages(&dummyNode1, interruptDummy);
262+
{
263+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
264+
peerLogic->SendMessages(&dummyNode1, interruptDummy);
265+
}
242266
BOOST_CHECK(!connman->IsBanned(addr1));
243267
{
244268
LOCK(cs_main);
245269
Misbehaving(dummyNode1.GetId(), 10);
246270
}
247-
peerLogic->SendMessages(&dummyNode1, interruptDummy);
271+
{
272+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
273+
peerLogic->SendMessages(&dummyNode1, interruptDummy);
274+
}
248275
BOOST_CHECK(!connman->IsBanned(addr1));
249276
{
250277
LOCK(cs_main);
251278
Misbehaving(dummyNode1.GetId(), 1);
252279
}
253-
peerLogic->SendMessages(&dummyNode1, interruptDummy);
280+
{
281+
LOCK2(cs_main, dummyNode1.cs_sendProcessing);
282+
peerLogic->SendMessages(&dummyNode1, interruptDummy);
283+
}
254284
BOOST_CHECK(connman->IsBanned(addr1));
255285
gArgs.ForceSetArg("-banscore", std::to_string(DEFAULT_BANSCORE_THRESHOLD));
256286

@@ -277,8 +307,10 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
277307
LOCK(cs_main);
278308
Misbehaving(dummyNode.GetId(), 100);
279309
}
280-
LOCK(dummyNode.cs_sendProcessing);
281-
peerLogic->SendMessages(&dummyNode, interruptDummy);
310+
{
311+
LOCK2(cs_main, dummyNode.cs_sendProcessing);
312+
peerLogic->SendMessages(&dummyNode, interruptDummy);
313+
}
282314
BOOST_CHECK(connman->IsBanned(addr));
283315

284316
SetMockTime(nStartTime+60*60);

0 commit comments

Comments
 (0)