Merge #9708: Clean up all known races/platform-specific UB at the time PR was opened

db2dc7a Move CNode::addrLocal access behind locked accessors (Matt Corallo)
036073b Move CNode::addrName accesses behind locked accessors (Matt Corallo)
d8f2b8a Make nTimeBestReceived atomic (Matt Corallo)
22b4966 Move [clean|str]SubVer writes/copyStats into a lock (Matt Corallo)
0f31872 Make nServices atomic (Matt Corallo)
96f42d8 Make nStartingHeight atomic (Matt Corallo)
512731b Access fRelayTxes with cs_filter lock in copyStats (Matt Corallo)
ae683c1 Avoid copying CNodeStats to make helgrind OK with buggy std::string (Matt Corallo)
644f123 Make nTimeConnected const in CNode (Matt Corallo)
321d0fc net: fix a few races. Credit @TheBlueMatt (Cory Fields)
This commit is contained in:
Pieter Wuille 2017-02-10 15:53:31 -08:00
commit a06ede9a13
No known key found for this signature in database
GPG Key ID: DBA1A67379A1A931
3 changed files with 105 additions and 45 deletions

View File

@ -164,8 +164,9 @@ int GetnScore(const CService& addr)
// Is our peer's addrLocal potentially useful as an external IP source? // Is our peer's addrLocal potentially useful as an external IP source?
bool IsPeerAddrLocalGood(CNode *pnode) bool IsPeerAddrLocalGood(CNode *pnode)
{ {
return fDiscover && pnode->addr.IsRoutable() && pnode->addrLocal.IsRoutable() && CService addrLocal = pnode->GetAddrLocal();
!IsLimited(pnode->addrLocal.GetNetwork()); return fDiscover && pnode->addr.IsRoutable() && addrLocal.IsRoutable() &&
!IsLimited(addrLocal.GetNetwork());
} }
// pushes our own address to a peer // pushes our own address to a peer
@ -180,7 +181,7 @@ void AdvertiseLocal(CNode *pnode)
if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() ||
GetRand((GetnScore(addrLocal) > LOCAL_MANUAL) ? 8:2) == 0)) GetRand((GetnScore(addrLocal) > LOCAL_MANUAL) ? 8:2) == 0))
{ {
addrLocal.SetIP(pnode->addrLocal); addrLocal.SetIP(pnode->GetAddrLocal());
} }
if (addrLocal.IsRoutable()) if (addrLocal.IsRoutable())
{ {
@ -307,9 +308,11 @@ CNode* CConnman::FindNode(const CSubNet& subNet)
CNode* CConnman::FindNode(const std::string& addrName) CNode* CConnman::FindNode(const std::string& addrName)
{ {
LOCK(cs_vNodes); LOCK(cs_vNodes);
BOOST_FOREACH(CNode* pnode, vNodes) BOOST_FOREACH(CNode* pnode, vNodes) {
if (pnode->addrName == addrName) if (pnode->GetAddrName() == addrName) {
return (pnode); return (pnode);
}
}
return NULL; return NULL;
} }
@ -373,9 +376,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
CNode* pnode = FindNode((CService)addrConnect); CNode* pnode = FindNode((CService)addrConnect);
if (pnode) if (pnode)
{ {
if (pnode->addrName.empty()) { pnode->MaybeSetAddrName(std::string(pszDest));
pnode->addrName = std::string(pszDest);
}
CloseSocket(hSocket); CloseSocket(hSocket);
LogPrintf("Failed to open new connection, already connected\n"); LogPrintf("Failed to open new connection, already connected\n");
return NULL; return NULL;
@ -389,7 +390,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); 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); CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, pszDest ? pszDest : "", false);
pnode->nServicesExpected = ServiceFlags(addrConnect.nServices & nRelevantServices); pnode->nServicesExpected = ServiceFlags(addrConnect.nServices & nRelevantServices);
pnode->nTimeConnected = GetSystemTimeInSeconds();
pnode->AddRef(); pnode->AddRef();
return pnode; return pnode;
@ -594,6 +594,33 @@ void CConnman::AddWhitelistedRange(const CSubNet &subnet) {
vWhitelistedRange.push_back(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;
}
}
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 #undef X
#define X(name) stats.name = name #define X(name) stats.name = name
void CNode::copyStats(CNodeStats &stats) void CNode::copyStats(CNodeStats &stats)
@ -601,21 +628,33 @@ void CNode::copyStats(CNodeStats &stats)
stats.nodeid = this->GetId(); stats.nodeid = this->GetId();
X(nServices); X(nServices);
X(addr); X(addr);
X(fRelayTxes); {
LOCK(cs_filter);
X(fRelayTxes);
}
X(nLastSend); X(nLastSend);
X(nLastRecv); X(nLastRecv);
X(nTimeConnected); X(nTimeConnected);
X(nTimeOffset); X(nTimeOffset);
X(addrName); stats.addrName = GetAddrName();
X(nVersion); X(nVersion);
X(cleanSubVer); {
LOCK(cs_SubVer);
X(cleanSubVer);
}
X(fInbound); X(fInbound);
X(fAddnode); X(fAddnode);
X(nStartingHeight); X(nStartingHeight);
X(nSendBytes); {
X(mapSendBytesPerMsgCmd); LOCK(cs_vSend);
X(nRecvBytes); X(mapSendBytesPerMsgCmd);
X(mapRecvBytesPerMsgCmd); X(nSendBytes);
}
{
LOCK(cs_vRecv);
X(mapRecvBytesPerMsgCmd);
X(nRecvBytes);
}
X(fWhitelisted); X(fWhitelisted);
// It is common for nodes with good ping times to suddenly become lagged, // It is common for nodes with good ping times to suddenly become lagged,
@ -635,7 +674,8 @@ void CNode::copyStats(CNodeStats &stats)
stats.dPingWait = (((double)nPingUsecWait) / 1e6); stats.dPingWait = (((double)nPingUsecWait) / 1e6);
// Leave string empty if addrLocal invalid (not filled in yet) // 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 #undef X
@ -643,6 +683,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete
{ {
complete = false; complete = false;
int64_t nTimeMicros = GetTimeMicros(); int64_t nTimeMicros = GetTimeMicros();
LOCK(cs_vRecv);
nLastRecv = nTimeMicros / 1000000; nLastRecv = nTimeMicros / 1000000;
nRecvBytes += nBytes; nRecvBytes += nBytes;
while (nBytes > 0) { while (nBytes > 0) {
@ -1786,8 +1827,9 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo()
if (pnode->addr.IsValid()) { if (pnode->addr.IsValid()) {
mapConnected[pnode->addr] = pnode->fInbound; mapConnected[pnode->addr] = pnode->fInbound;
} }
if (!pnode->addrName.empty()) { std::string addrName = pnode->GetAddrName();
mapConnectedByName[pnode->addrName] = std::make_pair(pnode->fInbound, static_cast<const CService&>(pnode->addr)); if (!addrName.empty()) {
mapConnectedByName[std::move(addrName)] = std::make_pair(pnode->fInbound, static_cast<const CService&>(pnode->addr));
} }
} }
} }
@ -2414,9 +2456,8 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats)
vstats.reserve(vNodes.size()); vstats.reserve(vNodes.size());
for(std::vector<CNode*>::iterator it = vNodes.begin(); it != vNodes.end(); ++it) { for(std::vector<CNode*>::iterator it = vNodes.begin(); it != vNodes.end(); ++it) {
CNode* pnode = *it; CNode* pnode = *it;
CNodeStats stats; vstats.emplace_back();
pnode->copyStats(stats); pnode->copyStats(vstats.back());
vstats.push_back(stats);
} }
} }
@ -2568,6 +2609,7 @@ unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
unsigned int CConnman::GetSendBufferSize() const{ return nSendBufferMaxSize; } 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) : 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), addr(addrIn),
fInbound(fInboundIn), fInbound(fInboundIn),
id(idIn), id(idIn),
@ -2587,7 +2629,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
nLastRecv = 0; nLastRecv = 0;
nSendBytes = 0; nSendBytes = 0;
nRecvBytes = 0; nRecvBytes = 0;
nTimeConnected = GetSystemTimeInSeconds();
nTimeOffset = 0; nTimeOffset = 0;
addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn;
nVersion = 0; nVersion = 0;

View File

@ -564,7 +564,7 @@ class CNode
friend class CConnman; friend class CConnman;
public: public:
// socket // socket
ServiceFlags nServices; std::atomic<ServiceFlags> nServices;
ServiceFlags nServicesExpected; ServiceFlags nServicesExpected;
SOCKET hSocket; SOCKET hSocket;
size_t nSendSize; // total size of all vSendMsg entries size_t nSendSize; // total size of all vSendMsg entries
@ -573,6 +573,7 @@ public:
std::deque<std::vector<unsigned char>> vSendMsg; std::deque<std::vector<unsigned char>> vSendMsg;
CCriticalSection cs_vSend; CCriticalSection cs_vSend;
CCriticalSection cs_hSocket; CCriticalSection cs_hSocket;
CCriticalSection cs_vRecv;
CCriticalSection cs_vProcessMsg; CCriticalSection cs_vProcessMsg;
std::list<CNetMessage> vProcessMsg; std::list<CNetMessage> vProcessMsg;
@ -584,19 +585,18 @@ public:
uint64_t nRecvBytes; uint64_t nRecvBytes;
std::atomic<int> nRecvVersion; std::atomic<int> nRecvVersion;
int64_t nLastSend; std::atomic<int64_t> nLastSend;
int64_t nLastRecv; std::atomic<int64_t> nLastRecv;
int64_t nTimeConnected; const int64_t nTimeConnected;
int64_t nTimeOffset; std::atomic<int64_t> nTimeOffset;
const CAddress addr; const CAddress addr;
std::string addrName;
CService addrLocal;
std::atomic<int> nVersion; std::atomic<int> nVersion;
// strSubVer is whatever byte array we read from the wire. However, this field is intended // 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 // to be printed out, displayed to humans in various forms and so on. So we sanitize it and
// store the sanitized version in cleanSubVer. The original should be used when dealing with // 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. // the network or wire types and the cleaned string used when displayed or logged.
std::string strSubVer, cleanSubVer; std::string strSubVer, cleanSubVer;
CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
bool fWhitelisted; // This peer can bypass DoS banning. bool fWhitelisted; // This peer can bypass DoS banning.
bool fFeeler; // If true this node is being used as a short lived feeler. bool fFeeler; // If true this node is being used as a short lived feeler.
bool fOneShot; bool fOneShot;
@ -614,7 +614,7 @@ public:
CSemaphoreGrant grantOutbound; CSemaphoreGrant grantOutbound;
CCriticalSection cs_filter; CCriticalSection cs_filter;
CBloomFilter* pfilter; CBloomFilter* pfilter;
int nRefCount; std::atomic<int> nRefCount;
const NodeId id; const NodeId id;
const uint64_t nKeyedNetGroup; const uint64_t nKeyedNetGroup;
@ -627,7 +627,7 @@ protected:
public: public:
uint256 hashContinue; uint256 hashContinue;
int nStartingHeight; std::atomic<int> nStartingHeight;
// flood relay // flood relay
std::vector<CAddress> vAddrToSend; std::vector<CAddress> vAddrToSend;
@ -665,15 +665,15 @@ public:
// Ping time measurement: // Ping time measurement:
// The pong reply we're expecting, or 0 if no pong expected. // The pong reply we're expecting, or 0 if no pong expected.
uint64_t nPingNonceSent; std::atomic<uint64_t> nPingNonceSent;
// Time (in usec) the last ping was sent, or 0 if no ping was ever sent. // Time (in usec) the last ping was sent, or 0 if no ping was ever sent.
int64_t nPingUsecStart; std::atomic<int64_t> nPingUsecStart;
// Last measured round-trip time. // Last measured round-trip time.
int64_t nPingUsecTime; std::atomic<int64_t> nPingUsecTime;
// Best measured round-trip time. // Best measured round-trip time.
int64_t nMinPingUsecTime; std::atomic<int64_t> nMinPingUsecTime;
// Whether a ping is requested. // Whether a ping is requested.
bool fPingQueued; std::atomic<bool> fPingQueued;
// Minimum fee rate with which to filter inv's to this node // Minimum fee rate with which to filter inv's to this node
CAmount minFeeFilter; CAmount minFeeFilter;
CCriticalSection cs_feeFilter; CCriticalSection cs_feeFilter;
@ -694,6 +694,12 @@ private:
const int nMyStartingHeight; const int nMyStartingHeight;
int nSendVersion; int nSendVersion;
std::list<CNetMessage> vRecvMsg; // Used only by SocketHandler thread std::list<CNetMessage> vRecvMsg; // Used only by SocketHandler thread
mutable CCriticalSection cs_addrName;
std::string addrName;
CService addrLocal;
mutable CCriticalSection cs_addrLocal;
public: public:
NodeId GetId() const { NodeId GetId() const {
@ -727,6 +733,10 @@ public:
void SetSendVersion(int nVersionIn); void SetSendVersion(int nVersionIn);
int GetSendVersion() const; int GetSendVersion() const;
CService GetAddrLocal() const;
//! May not be called more than once
void SetAddrLocal(const CService& addrLocalIn);
CNode* AddRef() CNode* AddRef()
{ {
nRefCount++; nRefCount++;
@ -796,6 +806,10 @@ public:
{ {
return nLocalServices; return nLocalServices;
} }
std::string GetAddrName() const;
//! Sets the addrName only if it was not previously set
void MaybeSetAddrName(const std::string& addrNameIn);
}; };

View File

@ -36,7 +36,7 @@
# error "Bitcoin cannot be compiled without assertions." # error "Bitcoin cannot be compiled without assertions."
#endif #endif
int64_t nTimeBestReceived = 0; // Used only to inform the wallet of when we last received a block std::atomic<int64_t> nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block
struct IteratorComparator struct IteratorComparator
{ {
@ -264,7 +264,7 @@ void PushNodeVersion(CNode *pnode, CConnman& connman, int64_t nTime)
void InitializeNode(CNode *pnode, CConnman& connman) { void InitializeNode(CNode *pnode, CConnman& connman) {
CAddress addr = pnode->addr; CAddress addr = pnode->addr;
std::string addrName = pnode->addrName; std::string addrName = pnode->GetAddrName();
NodeId nodeid = pnode->GetId(); NodeId nodeid = pnode->GetId();
{ {
LOCK(cs_main); LOCK(cs_main);
@ -1211,6 +1211,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
int nVersion; int nVersion;
int nSendVersion; int nSendVersion;
std::string strSubVer; std::string strSubVer;
std::string cleanSubVer;
int nStartingHeight = -1; int nStartingHeight = -1;
bool fRelay = true; bool fRelay = true;
@ -1246,6 +1247,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
vRecv >> addrFrom >> nNonce; vRecv >> addrFrom >> nNonce;
if (!vRecv.empty()) { if (!vRecv.empty()) {
vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH);
cleanSubVer = SanitizeString(strSubVer);
} }
if (!vRecv.empty()) { if (!vRecv.empty()) {
vRecv >> nStartingHeight; vRecv >> nStartingHeight;
@ -1272,9 +1274,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
connman.PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK)); connman.PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));
pfrom->nServices = nServices; pfrom->nServices = nServices;
pfrom->addrLocal = addrMe; pfrom->SetAddrLocal(addrMe);
pfrom->strSubVer = strSubVer; {
pfrom->cleanSubVer = SanitizeString(strSubVer); LOCK(pfrom->cs_SubVer);
pfrom->strSubVer = strSubVer;
pfrom->cleanSubVer = cleanSubVer;
}
pfrom->nStartingHeight = nStartingHeight; pfrom->nStartingHeight = nStartingHeight;
pfrom->fClient = !(nServices & NODE_NETWORK); pfrom->fClient = !(nServices & NODE_NETWORK);
{ {
@ -1310,7 +1315,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString()); LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString());
pfrom->PushAddress(addr, insecure_rand); pfrom->PushAddress(addr, insecure_rand);
} else if (IsPeerAddrLocalGood(pfrom)) { } else if (IsPeerAddrLocalGood(pfrom)) {
addr.SetIP(pfrom->addrLocal); addr.SetIP(addrMe);
LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString()); LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString());
pfrom->PushAddress(addr, insecure_rand); pfrom->PushAddress(addr, insecure_rand);
} }
@ -1330,7 +1335,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
remoteAddr = ", peeraddr=" + pfrom->addr.ToString(); remoteAddr = ", peeraddr=" + pfrom->addr.ToString();
LogPrintf("receive version message: %s: version %d, blocks=%d, us=%s, peer=%d%s\n", 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, pfrom->nStartingHeight, addrMe.ToString(), pfrom->id,
remoteAddr); remoteAddr);
@ -2450,7 +2455,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
if (pingUsecTime > 0) { if (pingUsecTime > 0) {
// Successful ping time measurement, replace previous // Successful ping time measurement, replace previous
pfrom->nPingUsecTime = pingUsecTime; pfrom->nPingUsecTime = pingUsecTime;
pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime, pingUsecTime); pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime.load(), pingUsecTime);
} else { } else {
// This should never happen // This should never happen
sProblem = "Timing mishap"; sProblem = "Timing mishap";