From 321d0fc6b6624c65508f8b9059418cb936f0bbbe Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 6 Feb 2017 02:34:57 -0500 Subject: [PATCH 01/10] net: fix a few races. Credit @TheBlueMatt These are (afaik) all long-standing races or concurrent accesses. Going forward, we can clean these up so that they're not all individual atomic accesses. - Reintroduce cs_vRecv to guard receive-specific vars - Lock vRecv/vSend for CNodeStats - Make some vars atomic. - Only set the connection time in CNode's constructor so that it doesn't change --- src/net.cpp | 16 +++++++++++----- src/net.h | 19 ++++++++++--------- src/net_processing.cpp | 2 +- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 7c45cff1d..c96ca469f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -389,7 +389,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, pszDest ? pszDest : "", false); pnode->nServicesExpected = ServiceFlags(addrConnect.nServices & nRelevantServices); - pnode->nTimeConnected = GetSystemTimeInSeconds(); pnode->AddRef(); return pnode; @@ -612,10 +611,16 @@ void CNode::copyStats(CNodeStats &stats) X(fInbound); X(fAddnode); X(nStartingHeight); - X(nSendBytes); - X(mapSendBytesPerMsgCmd); - X(nRecvBytes); - X(mapRecvBytesPerMsgCmd); + { + LOCK(cs_vSend); + X(mapSendBytesPerMsgCmd); + X(nSendBytes); + } + { + LOCK(cs_vRecv); + X(mapRecvBytesPerMsgCmd); + X(nRecvBytes); + } X(fWhitelisted); // It is common for nodes with good ping times to suddenly become lagged, @@ -643,6 +648,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete { complete = false; int64_t nTimeMicros = GetTimeMicros(); + LOCK(cs_vRecv); nLastRecv = nTimeMicros / 1000000; nRecvBytes += nBytes; while (nBytes > 0) { diff --git a/src/net.h b/src/net.h index e5a19e0f4..89501c764 100644 --- a/src/net.h +++ b/src/net.h @@ -573,6 +573,7 @@ public: std::deque> vSendMsg; CCriticalSection cs_vSend; CCriticalSection cs_hSocket; + CCriticalSection cs_vRecv; CCriticalSection cs_vProcessMsg; std::list vProcessMsg; @@ -584,10 +585,10 @@ public: uint64_t nRecvBytes; std::atomic nRecvVersion; - int64_t nLastSend; - int64_t nLastRecv; + std::atomic nLastSend; + std::atomic nLastRecv; int64_t nTimeConnected; - int64_t nTimeOffset; + std::atomic nTimeOffset; const CAddress addr; std::string addrName; CService addrLocal; @@ -614,7 +615,7 @@ public: CSemaphoreGrant grantOutbound; CCriticalSection cs_filter; CBloomFilter* pfilter; - int nRefCount; + std::atomic nRefCount; const NodeId id; const uint64_t nKeyedNetGroup; @@ -665,15 +666,15 @@ public: // Ping time measurement: // The pong reply we're expecting, or 0 if no pong expected. - uint64_t nPingNonceSent; + std::atomic nPingNonceSent; // Time (in usec) the last ping was sent, or 0 if no ping was ever sent. - int64_t nPingUsecStart; + std::atomic nPingUsecStart; // Last measured round-trip time. - int64_t nPingUsecTime; + std::atomic nPingUsecTime; // Best measured round-trip time. - int64_t nMinPingUsecTime; + std::atomic nMinPingUsecTime; // Whether a ping is requested. - bool fPingQueued; + std::atomic fPingQueued; // Minimum fee rate with which to filter inv's to this node CAmount minFeeFilter; CCriticalSection cs_feeFilter; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bb14e69d8..e89a897bd 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2450,7 +2450,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (pingUsecTime > 0) { // Successful ping time measurement, replace previous pfrom->nPingUsecTime = pingUsecTime; - pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime, pingUsecTime); + pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime); } else { // This should never happen sProblem = "Timing mishap"; From 644f1234e22626a7b5618a1dae60a8457a4063b1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:42:49 -0500 Subject: [PATCH 02/10] Make nTimeConnected const in CNode --- src/net.cpp | 2 +- src/net.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index c96ca469f..0e6e00d58 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2574,6 +2574,7 @@ unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } unsigned int CConnman::GetSendBufferSize() const{ return nSendBufferMaxSize; } CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const std::string& addrNameIn, bool fInboundIn) : + nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), fInbound(fInboundIn), id(idIn), @@ -2593,7 +2594,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn nLastRecv = 0; nSendBytes = 0; nRecvBytes = 0; - nTimeConnected = GetSystemTimeInSeconds(); nTimeOffset = 0; addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; nVersion = 0; diff --git a/src/net.h b/src/net.h index 89501c764..4bbcfac88 100644 --- a/src/net.h +++ b/src/net.h @@ -587,7 +587,7 @@ public: std::atomic nLastSend; std::atomic nLastRecv; - int64_t nTimeConnected; + const int64_t nTimeConnected; std::atomic nTimeOffset; const CAddress addr; std::string addrName; From ae683c1b1960b32134f5a5a29504691c91f39cf3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:44:38 -0500 Subject: [PATCH 03/10] Avoid copying CNodeStats to make helgrind OK with buggy std::string --- src/net.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0e6e00d58..b7243dce2 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2420,9 +2420,8 @@ void CConnman::GetNodeStats(std::vector& vstats) vstats.reserve(vNodes.size()); for(std::vector::iterator it = vNodes.begin(); it != vNodes.end(); ++it) { CNode* pnode = *it; - CNodeStats stats; - pnode->copyStats(stats); - vstats.push_back(stats); + vstats.emplace_back(); + pnode->copyStats(vstats.back()); } } From 512731bed0782f10092de35a960153b17ecc11eb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:53:34 -0500 Subject: [PATCH 04/10] Access fRelayTxes with cs_filter lock in copyStats --- src/net.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index b7243dce2..ea8a2a0a4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -600,7 +600,10 @@ void CNode::copyStats(CNodeStats &stats) stats.nodeid = this->GetId(); X(nServices); X(addr); - X(fRelayTxes); + { + LOCK(cs_filter); + X(fRelayTxes); + } X(nLastSend); X(nLastRecv); X(nTimeConnected); From 96f42d8a12871b8d5c4e31fd27d8135f97c6b3e0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:15:30 -0500 Subject: [PATCH 05/10] Make nStartingHeight atomic --- src/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.h b/src/net.h index 4bbcfac88..566284c70 100644 --- a/src/net.h +++ b/src/net.h @@ -628,7 +628,7 @@ protected: public: uint256 hashContinue; - int nStartingHeight; + std::atomic nStartingHeight; // flood relay std::vector vAddrToSend; From 0f3187261519c7568ef4211ce12b9740a3c1200f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:20:16 -0500 Subject: [PATCH 06/10] Make nServices atomic --- src/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.h b/src/net.h index 566284c70..cf742a8ca 100644 --- a/src/net.h +++ b/src/net.h @@ -564,7 +564,7 @@ class CNode friend class CConnman; public: // socket - ServiceFlags nServices; + std::atomic nServices; ServiceFlags nServicesExpected; SOCKET hSocket; size_t nSendSize; // total size of all vSendMsg entries From 22b4966a29501c4f3f2e970ac5008fbd91e665a9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:08:31 -0500 Subject: [PATCH 07/10] Move [clean|str]SubVer writes/copyStats into a lock --- src/net.cpp | 5 ++++- src/net.h | 1 + src/net_processing.cpp | 11 ++++++++--- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index ea8a2a0a4..e7521f86d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -610,7 +610,10 @@ void CNode::copyStats(CNodeStats &stats) X(nTimeOffset); X(addrName); X(nVersion); - X(cleanSubVer); + { + LOCK(cs_SubVer); + X(cleanSubVer); + } X(fInbound); X(fAddnode); X(nStartingHeight); diff --git a/src/net.h b/src/net.h index cf742a8ca..ddc050eb1 100644 --- a/src/net.h +++ b/src/net.h @@ -598,6 +598,7 @@ public: // store the sanitized version in cleanSubVer. The original should be used when dealing with // the network or wire types and the cleaned string used when displayed or logged. std::string strSubVer, cleanSubVer; + CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer bool fWhitelisted; // This peer can bypass DoS banning. bool fFeeler; // If true this node is being used as a short lived feeler. bool fOneShot; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e89a897bd..b5feac2d5 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1211,6 +1211,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr int nVersion; int nSendVersion; std::string strSubVer; + std::string cleanSubVer; int nStartingHeight = -1; bool fRelay = true; @@ -1246,6 +1247,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr vRecv >> addrFrom >> nNonce; if (!vRecv.empty()) { vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); + cleanSubVer = SanitizeString(strSubVer); } if (!vRecv.empty()) { vRecv >> nStartingHeight; @@ -1273,8 +1275,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr pfrom->nServices = nServices; pfrom->addrLocal = addrMe; - pfrom->strSubVer = strSubVer; - pfrom->cleanSubVer = SanitizeString(strSubVer); + { + LOCK(pfrom->cs_SubVer); + pfrom->strSubVer = strSubVer; + pfrom->cleanSubVer = cleanSubVer; + } pfrom->nStartingHeight = nStartingHeight; pfrom->fClient = !(nServices & NODE_NETWORK); { @@ -1330,7 +1335,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr remoteAddr = ", peeraddr=" + pfrom->addr.ToString(); LogPrintf("receive version message: %s: version %d, blocks=%d, us=%s, peer=%d%s\n", - pfrom->cleanSubVer, pfrom->nVersion, + cleanSubVer, pfrom->nVersion, pfrom->nStartingHeight, addrMe.ToString(), pfrom->id, remoteAddr); From d8f2b8a8c032b83a3bd90750e58abaeece7e34e7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 17:38:57 -0500 Subject: [PATCH 08/10] Make nTimeBestReceived atomic --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b5feac2d5..62397e68c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -36,7 +36,7 @@ # error "Bitcoin cannot be compiled without assertions." #endif -int64_t nTimeBestReceived = 0; // Used only to inform the wallet of when we last received a block +std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block struct IteratorComparator { From 036073bf87c07f8d69e39168dd93a52f1aafe85c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:04:34 -0500 Subject: [PATCH 09/10] Move CNode::addrName accesses behind locked accessors --- src/net.cpp | 30 ++++++++++++++++++++++-------- src/net.h | 8 +++++++- src/net_processing.cpp | 2 +- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index e7521f86d..8aa126198 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -307,9 +307,11 @@ CNode* CConnman::FindNode(const CSubNet& subNet) CNode* CConnman::FindNode(const std::string& addrName) { LOCK(cs_vNodes); - BOOST_FOREACH(CNode* pnode, vNodes) - if (pnode->addrName == addrName) + BOOST_FOREACH(CNode* pnode, vNodes) { + if (pnode->GetAddrName() == addrName) { return (pnode); + } + } return NULL; } @@ -373,9 +375,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo CNode* pnode = FindNode((CService)addrConnect); if (pnode) { - if (pnode->addrName.empty()) { - pnode->addrName = std::string(pszDest); - } + pnode->MaybeSetAddrName(std::string(pszDest)); CloseSocket(hSocket); LogPrintf("Failed to open new connection, already connected\n"); return NULL; @@ -593,6 +593,19 @@ void CConnman::AddWhitelistedRange(const CSubNet &subnet) { vWhitelistedRange.push_back(subnet); } + +std::string CNode::GetAddrName() const { + LOCK(cs_addrName); + return addrName; +} + +void CNode::MaybeSetAddrName(const std::string& addrNameIn) { + LOCK(cs_addrName); + if (addrName.empty()) { + addrName = addrNameIn; + } +} + #undef X #define X(name) stats.name = name void CNode::copyStats(CNodeStats &stats) @@ -608,7 +621,7 @@ void CNode::copyStats(CNodeStats &stats) X(nLastRecv); X(nTimeConnected); X(nTimeOffset); - X(addrName); + stats.addrName = GetAddrName(); X(nVersion); { LOCK(cs_SubVer); @@ -1798,8 +1811,9 @@ std::vector CConnman::GetAddedNodeInfo() if (pnode->addr.IsValid()) { mapConnected[pnode->addr] = pnode->fInbound; } - if (!pnode->addrName.empty()) { - mapConnectedByName[pnode->addrName] = std::make_pair(pnode->fInbound, static_cast(pnode->addr)); + std::string addrName = pnode->GetAddrName(); + if (!addrName.empty()) { + mapConnectedByName[std::move(addrName)] = std::make_pair(pnode->fInbound, static_cast(pnode->addr)); } } } diff --git a/src/net.h b/src/net.h index ddc050eb1..2cfc74e3d 100644 --- a/src/net.h +++ b/src/net.h @@ -590,7 +590,6 @@ public: const int64_t nTimeConnected; std::atomic nTimeOffset; const CAddress addr; - std::string addrName; CService addrLocal; std::atomic nVersion; // strSubVer is whatever byte array we read from the wire. However, this field is intended @@ -696,6 +695,9 @@ private: const int nMyStartingHeight; int nSendVersion; std::list vRecvMsg; // Used only by SocketHandler thread + + mutable CCriticalSection cs_addrName; + std::string addrName; public: NodeId GetId() const { @@ -798,6 +800,10 @@ public: { return nLocalServices; } + + std::string GetAddrName() const; + //! Sets the addrName only if it was not previously set + void MaybeSetAddrName(const std::string& addrNameIn); }; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 62397e68c..b0c9b3c71 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -264,7 +264,7 @@ void PushNodeVersion(CNode *pnode, CConnman& connman, int64_t nTime) void InitializeNode(CNode *pnode, CConnman& connman) { CAddress addr = pnode->addr; - std::string addrName = pnode->addrName; + std::string addrName = pnode->GetAddrName(); NodeId nodeid = pnode->GetId(); { LOCK(cs_main); From db2dc7a58cb0a3df58188b748df8e0d04ba76f00 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:18:51 -0500 Subject: [PATCH 10/10] Move CNode::addrLocal access behind locked accessors --- src/net.cpp | 24 ++++++++++++++++++++---- src/net.h | 8 +++++++- src/net_processing.cpp | 4 ++-- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 8aa126198..505eb971c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -164,8 +164,9 @@ int GetnScore(const CService& addr) // Is our peer's addrLocal potentially useful as an external IP source? bool IsPeerAddrLocalGood(CNode *pnode) { - return fDiscover && pnode->addr.IsRoutable() && pnode->addrLocal.IsRoutable() && - !IsLimited(pnode->addrLocal.GetNetwork()); + CService addrLocal = pnode->GetAddrLocal(); + return fDiscover && pnode->addr.IsRoutable() && addrLocal.IsRoutable() && + !IsLimited(addrLocal.GetNetwork()); } // pushes our own address to a peer @@ -180,7 +181,7 @@ void AdvertiseLocal(CNode *pnode) if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || GetRand((GetnScore(addrLocal) > LOCAL_MANUAL) ? 8:2) == 0)) { - addrLocal.SetIP(pnode->addrLocal); + addrLocal.SetIP(pnode->GetAddrLocal()); } if (addrLocal.IsRoutable()) { @@ -606,6 +607,20 @@ void CNode::MaybeSetAddrName(const std::string& addrNameIn) { } } +CService CNode::GetAddrLocal() const { + LOCK(cs_addrLocal); + return addrLocal; +} + +void CNode::SetAddrLocal(const CService& addrLocalIn) { + LOCK(cs_addrLocal); + if (addrLocal.IsValid()) { + error("Addr local already set for node: %i. Refusing to change from %s to %s", id, addrLocal.ToString(), addrLocalIn.ToString()); + } else { + addrLocal = addrLocalIn; + } +} + #undef X #define X(name) stats.name = name void CNode::copyStats(CNodeStats &stats) @@ -659,7 +674,8 @@ void CNode::copyStats(CNodeStats &stats) stats.dPingWait = (((double)nPingUsecWait) / 1e6); // Leave string empty if addrLocal invalid (not filled in yet) - stats.addrLocal = addrLocal.IsValid() ? addrLocal.ToString() : ""; + CService addrLocalUnlocked = GetAddrLocal(); + stats.addrLocal = addrLocalUnlocked.IsValid() ? addrLocalUnlocked.ToString() : ""; } #undef X diff --git a/src/net.h b/src/net.h index 2cfc74e3d..29b6a44c8 100644 --- a/src/net.h +++ b/src/net.h @@ -590,7 +590,6 @@ public: const int64_t nTimeConnected; std::atomic nTimeOffset; const CAddress addr; - CService addrLocal; std::atomic nVersion; // strSubVer is whatever byte array we read from the wire. However, this field is intended // to be printed out, displayed to humans in various forms and so on. So we sanitize it and @@ -698,6 +697,9 @@ private: mutable CCriticalSection cs_addrName; std::string addrName; + + CService addrLocal; + mutable CCriticalSection cs_addrLocal; public: NodeId GetId() const { @@ -731,6 +733,10 @@ public: void SetSendVersion(int nVersionIn); int GetSendVersion() const; + CService GetAddrLocal() const; + //! May not be called more than once + void SetAddrLocal(const CService& addrLocalIn); + CNode* AddRef() { nRefCount++; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b0c9b3c71..7d76aa0b4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1274,7 +1274,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr connman.PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK)); pfrom->nServices = nServices; - pfrom->addrLocal = addrMe; + pfrom->SetAddrLocal(addrMe); { LOCK(pfrom->cs_SubVer); pfrom->strSubVer = strSubVer; @@ -1315,7 +1315,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString()); pfrom->PushAddress(addr, insecure_rand); } else if (IsPeerAddrLocalGood(pfrom)) { - addr.SetIP(pfrom->addrLocal); + addr.SetIP(addrMe); LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString()); pfrom->PushAddress(addr, insecure_rand); }