From 5c3caa46b2a92dec88db70971dd5d5535a080f05 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 4 Aug 2020 01:37:00 -0300 Subject: [PATCH] lock cs_main for Misbehaving Github-Pull: #1785 Rebased-From: bc04b4db232d344572b8bc2c6a41b197d8cb48df --- src/main.cpp | 14 +++++++++----- src/main.h | 2 +- src/masternode-budget.cpp | 3 +++ src/masternode-payments.cpp | 2 ++ src/masternodeman.cpp | 12 +++++++++--- src/test/DoS_tests.cpp | 19 ++++++++++++------- 6 files changed, 36 insertions(+), 16 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index d1093215589a6..10f505a3c4618 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1643,7 +1643,7 @@ void CheckForkWarningConditionsOnNewFork(CBlockIndex* pindexNewForkTip) } // Requires cs_main. -void Misbehaving(NodeId pnode, int howmuch) +void Misbehaving(NodeId pnode, int howmuch) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { if (howmuch == 0) return; @@ -1687,8 +1687,10 @@ void static InvalidBlockFound(CBlockIndex* pindex, const CValidationState& state assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes CBlockReject reject = {(unsigned char) state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), pindex->GetBlockHash()}; State(it->second)->rejects.push_back(reject); - if (nDoS > 0) + if (nDoS > 0) { + LOCK(cs_main); Misbehaving(it->second, nDoS); + } } } if (!state.CorruptionPossible()) { @@ -5214,9 +5216,11 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR pfrom->fClient = !(pfrom->nServices & NODE_NETWORK); - // Potentially mark this peer as a preferred download peer. - UpdatePreferredDownload(pfrom, State(pfrom->GetId())); - + { + LOCK(cs_main); + // Potentially mark this peer as a preferred download peer. + UpdatePreferredDownload(pfrom, State(pfrom->GetId())); + } // Change version pfrom->PushMessage(NetMsgType::VERACK); pfrom->ssSend.SetVersion(std::min(pfrom->nVersion, PROTOCOL_VERSION)); diff --git a/src/main.h b/src/main.h index bf1a665d229fe..7a455cbce9716 100644 --- a/src/main.h +++ b/src/main.h @@ -239,7 +239,7 @@ CBlockIndex* InsertBlockIndex(uint256 hash); /** Get statistics from node state */ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats); /** Increase a node's misbehavior score. */ -void Misbehaving(NodeId nodeid, int howmuch); +void Misbehaving(NodeId nodeid, int howmuch) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Flush all state, indexes and buffers to disk. */ void FlushStateToDisk(); diff --git a/src/masternode-budget.cpp b/src/masternode-budget.cpp index c62cc51ec9f1f..210189e1a2948 100644 --- a/src/masternode-budget.cpp +++ b/src/masternode-budget.cpp @@ -1080,6 +1080,7 @@ void CBudgetManager::ProcessMessage(CNode* pfrom, std::string& strCommand, CData if (nProp.IsNull()) { if (pfrom->HasFulfilledRequest("budgetvotesync")) { LogPrint(BCLog::MNBUDGET,"mnvs - peer already asked me for the list\n"); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); return; } @@ -1149,6 +1150,7 @@ void CBudgetManager::ProcessMessage(CNode* pfrom, std::string& strCommand, CData if (!vote.CheckSignature()) { if (masternodeSync.IsSynced()) { LogPrintf("CBudgetManager::ProcessMessage() : mvote - signature invalid\n"); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); } // it could just be a non-synced masternode @@ -1223,6 +1225,7 @@ void CBudgetManager::ProcessMessage(CNode* pfrom, std::string& strCommand, CData if (!vote.CheckSignature()) { if (masternodeSync.IsSynced()) { LogPrintf("CBudgetManager::ProcessMessage() : fbvote - signature from masternode %s invalid\n", HexStr(pmn->pubKeyMasternode)); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); } // it could just be a non-synced masternode diff --git a/src/masternode-payments.cpp b/src/masternode-payments.cpp index a8471a581cc9b..ac0f1d5de6eb1 100644 --- a/src/masternode-payments.cpp +++ b/src/masternode-payments.cpp @@ -430,6 +430,7 @@ void CMasternodePayments::ProcessMessageMasternodePayments(CNode* pfrom, std::st if (Params().NetworkID() == CBaseChainParams::MAIN) { if (pfrom->HasFulfilledRequest(NetMsgType::GETMNWINNERS)) { LogPrintf("CMasternodePayments::ProcessMessageMasternodePayments() : mnget - peer already asked me for the list\n"); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); return; } @@ -484,6 +485,7 @@ void CMasternodePayments::ProcessMessageMasternodePayments(CNode* pfrom, std::st if (!winner.CheckSignature()) { if (masternodeSync.IsSynced()) { LogPrintf("CMasternodePayments::ProcessMessageMasternodePayments() : mnw - invalid signature\n"); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); } // it could just be a non-synced masternode diff --git a/src/masternodeman.cpp b/src/masternodeman.cpp index 6dee0c58314a3..02888263b25e2 100644 --- a/src/masternodeman.cpp +++ b/src/masternodeman.cpp @@ -700,9 +700,10 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData int nDoS = 0; if (!mnb.CheckAndUpdate(nDoS)) { - if (nDoS > 0) + if (nDoS > 0) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), nDoS); - + } //failed return; } @@ -711,6 +712,7 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData // - this is expensive, so it's only done once per Masternode if (!mnb.IsInputAssociatedWithPubkey()) { LogPrintf("CMasternodeMan::ProcessMessage() : mnb - Got mismatched pubkey and vin\n"); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 33); return; } @@ -724,8 +726,10 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData } else { LogPrint(BCLog::MASTERNODE,"mnb - Rejected Masternode entry %s\n", mnb.vin.prevout.hash.ToString()); - if (nDoS > 0) + if (nDoS > 0) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), nDoS); + } } } @@ -743,6 +747,7 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData if (nDoS > 0) { // if anything significant failed, mark that node + LOCK(cs_main); Misbehaving(pfrom->GetId(), nDoS); } else { // if nothing significant failed, search existing Masternode list @@ -770,6 +775,7 @@ void CMasternodeMan::ProcessMessage(CNode* pfrom, std::string& strCommand, CData int64_t t = (*i).second; if (GetTime() < t) { LogPrintf("CMasternodeMan::ProcessMessage() : dseg - peer already asked me for the list\n"); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 34); return; } diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index b2e5cc3fc0e51..7a1dc00ddc459 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -45,13 +45,18 @@ CService ip(uint32_t i) BOOST_FIXTURE_TEST_SUITE(DoS_tests, TestingSetup) +void misbehave(NodeId id, int value) { + LOCK(cs_main); + Misbehaving(id, value); // Should get banned +} + BOOST_AUTO_TEST_CASE(DoS_banning) { CNode::ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); CNode dummyNode1(INVALID_SOCKET, addr1, "", true); dummyNode1.nVersion = 1; - Misbehaving(dummyNode1.GetId(), 100); // Should get banned + misbehave(dummyNode1.GetId(), 100); // Should get banned SendMessages(&dummyNode1); BOOST_CHECK(CNode::IsBanned(addr1)); BOOST_CHECK(!CNode::IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned @@ -59,11 +64,11 @@ BOOST_AUTO_TEST_CASE(DoS_banning) CAddress addr2(ip(0xa0b0c002), NODE_NONE); CNode dummyNode2(INVALID_SOCKET, addr2, "", true); dummyNode2.nVersion = 1; - Misbehaving(dummyNode2.GetId(), 50); + misbehave(dummyNode2.GetId(), 50); SendMessages(&dummyNode2); BOOST_CHECK(!CNode::IsBanned(addr2)); // 2 not banned yet... BOOST_CHECK(CNode::IsBanned(addr1)); // ... but 1 still should be - Misbehaving(dummyNode2.GetId(), 50); + misbehave(dummyNode2.GetId(), 50); SendMessages(&dummyNode2); BOOST_CHECK(CNode::IsBanned(addr2)); } @@ -75,13 +80,13 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) CAddress addr1(ip(0xa0b0c001), NODE_NONE); CNode dummyNode1(INVALID_SOCKET, addr1, "", true); dummyNode1.nVersion = 1; - Misbehaving(dummyNode1.GetId(), 100); + misbehave(dummyNode1.GetId(), 100); SendMessages(&dummyNode1); BOOST_CHECK(!CNode::IsBanned(addr1)); - Misbehaving(dummyNode1.GetId(), 10); + misbehave(dummyNode1.GetId(), 10); SendMessages(&dummyNode1); BOOST_CHECK(!CNode::IsBanned(addr1)); - Misbehaving(dummyNode1.GetId(), 1); + misbehave(dummyNode1.GetId(), 1); SendMessages(&dummyNode1); BOOST_CHECK(CNode::IsBanned(addr1)); mapArgs.erase("-banscore"); @@ -97,7 +102,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) CNode dummyNode(INVALID_SOCKET, addr, "", true); dummyNode.nVersion = 1; - Misbehaving(dummyNode.GetId(), 100); + misbehave(dummyNode.GetId(), 100); SendMessages(&dummyNode); BOOST_CHECK(CNode::IsBanned(addr));