From 290fb3b57e33999c8082d6b9aa40fd31d94e89bb Mon Sep 17 00:00:00 2001 From: Oleg Girko Date: Mon, 17 Jul 2017 18:49:17 +0100 Subject: [PATCH] Backport Bitcoin PR#7942: locking for Misbehave() and other cs_main locking fixes (#1535) * lock cs_main for chainActive ActivateBestChain uses chainActive after releasing the lock; reorder operations to move all access to synchronized object into existing LOCK(cs_main) block. * lock cs_main for State/Misbehaving ProcessMessage calls State(...) and Misbehaving(...) without holding the required lock; add LOCK(cs_main) blocks. --- src/main.cpp | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index eec4c6edddad3..4588432b87640 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3319,14 +3319,15 @@ static void NotifyHeaderTip() { */ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, const CBlock *pblock) { CBlockIndex *pindexMostWork = NULL; + CBlockIndex *pindexNewTip = NULL; do { boost::this_thread::interruption_point(); if (ShutdownRequested()) break; - CBlockIndex *pindexNewTip = NULL; const CBlockIndex *pindexFork; bool fInitialDownload; + int nNewHeight; { LOCK(cs_main); CBlockIndex *pindexOldTip = chainActive.Tip(); @@ -3349,6 +3350,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, pindexNewTip = chainActive.Tip(); pindexFork = chainActive.FindFork(pindexOldTip); fInitialDownload = IsInitialBlockDownload(); + nNewHeight = chainActive.Height(); } // When we reach this point, we switched to a new tip (stored in pindexNewTip). @@ -3377,7 +3379,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, { LOCK(cs_vNodes); BOOST_FOREACH(CNode* pnode, vNodes) { - if (chainActive.Height() > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) { + if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) { BOOST_REVERSE_FOREACH(const uint256& hash, vHashes) { pnode->PushBlockHash(hash); } @@ -3390,7 +3392,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, } } } - } while(pindexMostWork != chainActive.Tip()); + } while (pindexNewTip != pindexMostWork); CheckBlockIndex(chainparams.GetConsensus()); // Write changes periodically to disk, after relay. @@ -5273,6 +5275,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, strCommand == NetMsgType::FILTERCLEAR)) { if (pfrom->nVersion >= NO_BLOOM_VERSION) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 100); return false; } else if (GetBoolArg("-enforcenodebloom", false)) { @@ -5294,6 +5297,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, if (pfrom->nVersion != 0) { pfrom->PushMessage(NetMsgType::REJECT, strCommand, REJECT_DUPLICATE, string("Duplicate version message")); + LOCK(cs_main); Misbehaving(pfrom->GetId(), 1); return false; } @@ -5363,16 +5367,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, pfrom->fClient = !(pfrom->nServices & NODE_NETWORK); - CNodeState* pNodeState = NULL; + // Potentially mark this peer as a preferred download peer. { - LOCK(cs_main); - pNodeState = State(pfrom->GetId()); - assert(pNodeState); + LOCK(cs_main); + UpdatePreferredDownload(pfrom, State(pfrom->GetId())); } - // Potentially mark this peer as a preferred download peer. - UpdatePreferredDownload(pfrom, pNodeState); - // Change version pfrom->PushMessage(NetMsgType::VERACK); pfrom->ssSend.SetVersion(min(pfrom->nVersion, PROTOCOL_VERSION)); @@ -5436,6 +5436,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, else if (pfrom->nVersion == 0) { // Must have a version message before anything else + LOCK(cs_main); Misbehaving(pfrom->GetId(), 1); return false; } @@ -5471,6 +5472,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; if (vAddr.size() > 1000) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); return error("message addr size() = %u", vAddr.size()); } @@ -5543,6 +5545,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); return error("message inv size() = %u", vInv.size()); } @@ -5624,6 +5627,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, vRecv >> vInv; if (vInv.size() > MAX_INV_SZ) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); return error("message getdata size() = %u", vInv.size()); } @@ -5955,6 +5959,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks. unsigned int nCount = ReadCompactSize(vRecv); if (nCount > MAX_HEADERS_RESULTS) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 20); return error("headers message size = %u", nCount); } @@ -6252,8 +6257,11 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, vRecv >> filter; if (!filter.IsWithinSizeConstraints()) + { // There is no excuse for sending a too-large filter + LOCK(cs_main); Misbehaving(pfrom->GetId(), 100); + } else { LOCK(pfrom->cs_filter); @@ -6274,13 +6282,17 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // and thus, the maximum size any matched object can have) in a filteradd message if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 100); } else { LOCK(pfrom->cs_filter); if (pfrom->pfilter) pfrom->pfilter->insert(vData); else + { + LOCK(cs_main); Misbehaving(pfrom->GetId(), 100); + } } }