Skip to content

Commit

Permalink
Backport Bitcoin PR#7942: locking for Misbehave() and other cs_main l…
Browse files Browse the repository at this point in the history 
…ocking 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.
  • Loading branch information
@OlegGirko @UdjinM6
OlegGirko authored and UdjinM6 committed Jul 17, 2017
1 parent 8075370 commit 290fb3b
Showing 1 changed file with 22 additions and 10 deletions.
32 changes: 22 additions & 10 deletions src/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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).

Expand Down Expand Up @@ -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);
}
Expand All @@ -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.
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}
}

Expand Down

0 comments on commit 290fb3b

Please sign in to comment.