From 321d0fc6b6624c65508f8b9059418cb936f0bbbe Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 6 Feb 2017 02:34:57 -0500 Subject: 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 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'src/net.cpp') 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) { -- cgit v1.2.3 From 644f1234e22626a7b5618a1dae60a8457a4063b1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:42:49 -0500 Subject: Make nTimeConnected const in CNode --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/net.cpp') 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; -- cgit v1.2.3 From ae683c1b1960b32134f5a5a29504691c91f39cf3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:44:38 -0500 Subject: Avoid copying CNodeStats to make helgrind OK with buggy std::string --- src/net.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'src/net.cpp') 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()); } } -- cgit v1.2.3 From 512731bed0782f10092de35a960153b17ecc11eb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 11:53:34 -0500 Subject: Access fRelayTxes with cs_filter lock in copyStats --- src/net.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/net.cpp') 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); -- cgit v1.2.3 From 22b4966a29501c4f3f2e970ac5008fbd91e665a9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:08:31 -0500 Subject: Move [clean|str]SubVer writes/copyStats into a lock --- src/net.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/net.cpp') 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); -- cgit v1.2.3 From 036073bf87c07f8d69e39168dd93a52f1aafe85c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:04:34 -0500 Subject: Move CNode::addrName accesses behind locked accessors --- src/net.cpp | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) (limited to 'src/net.cpp') 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)); } } } -- cgit v1.2.3 From db2dc7a58cb0a3df58188b748df8e0d04ba76f00 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 12:18:51 -0500 Subject: Move CNode::addrLocal access behind locked accessors --- src/net.cpp | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) (limited to 'src/net.cpp') 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 -- cgit v1.2.3