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_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/net_processing.cpp') 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"; -- 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_processing.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'src/net_processing.cpp') 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); -- cgit v1.2.3 From d8f2b8a8c032b83a3bd90750e58abaeece7e34e7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Feb 2017 17:38:57 -0500 Subject: Make nTimeBestReceived atomic --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/net_processing.cpp') 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 { -- 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_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/net_processing.cpp') 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); -- 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_processing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/net_processing.cpp') 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); } -- cgit v1.2.3