net: fix a few races. Credit @TheBlueMatt #9695
Conversation
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
| @@ -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(); | |||
There was a problem hiding this comment.
It was redundant. It's additionally set in CNode's ctor.
| @@ -642,6 +647,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete | |||
| { | |||
| complete = false; | |||
| int64_t nTimeMicros = GetTimeMicros(); | |||
| LOCK(cs_vRecv); | |||
There was a problem hiding this comment.
Did you mean to protect the entire receive loop below with this lock?
There was a problem hiding this comment.
Just trying to use a big hammer here for 0.14. The lock only contends with copyStats.
|
Hmm, this actually needs a lot more to make copyStats non-racy, and I think we really need to do them - we're copying std::strings concurrently right now. |
|
Pushed a few more at @TheBlueMatt's request. This is essentially his PR now, just re-using this one rather than opening another. Implied ACK, btw. |
|
The above set of commits fixes all the races I'm aware of with the exception of CNode::addrLocal and CNode::addrName. Both of which I proposed fixing in a separate PR since my preferred fix is a (tiny bit) more involved (just putting accessors around them). |
|
#9695 is a superset of this, and those who have acked here have also acked there. Closing in favor. |
|
@dcousens haha, let's say I was trying to bring github down with a recursive link attack :p Busted c/p. Yes, 9708. Thanks. |
Note that we still need a fix for hSocket for 0.14. Still discussing the optimal approach.
These are (afaik) all long-standing races or concurrent accesses. Going forward (post 0.14), we can clean these up so that they're not all individual atomic accesses.