net: fix a few races. Credit @TheBlueMatt #9695

pull theuni wants to merge 8 commits into bitcoin:master from theuni:net-atomic changing 3 files +46 −28

theuni commented at 8:18 AM on February 6, 2017: member
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.
- 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

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

1078275b50

fanquake added the label P2P on Feb 6, 2017

in src/net.cpp:None in 1078275b50 outdated

 388 | @@ -389,7 +389,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 389 |          uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
 390 |          CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, pszDest ? pszDest : "", false);
 391 |          pnode->nServicesExpected = ServiceFlags(addrConnect.nServices & nRelevantServices);
 392 | -        pnode->nTimeConnected = GetSystemTimeInSeconds();

sipa commented at 8:21 AM on February 6, 2017:

Did this move?

theuni commented at 9:05 AM on February 6, 2017:

It was redundant. It's additionally set in CNode's ctor.

Make nTimeConnected const in CNode 69e46f017d
Avoid copying CNodeStats to make helgrind OK with buggy std::string 15efb59a35
Access fRelayTxes with cs_filter lock in copyStats fe2cec8395
in src/net.cpp:None in 1078275b50 outdated
```
 646 | @@ -642,6 +647,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete
 647 |  {
 648 |      complete = false;
 649 |      int64_t nTimeMicros = GetTimeMicros();
 650 | +    LOCK(cs_vRecv);
```
sipa commented at 8:23 AM on February 6, 2017:

Did you mean to protect the entire receive loop below with this lock?

theuni commented at 9:09 AM on February 6, 2017:

Just trying to use a big hammer here for 0.14. The lock only contends with copyStats.
TheBlueMatt commented at 5:58 PM on February 6, 2017: member

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.
Make nStartingHeight atomic cbfa270bb7
Make nServices atomic 29113eb78e
Move [clean|str]SubVer writes/copyStats into a lock 7706f6327c
Make nTimeBestReceived atomic 7d4250c670
theuni commented at 11:54 PM on February 6, 2017: member

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.
TheBlueMatt commented at 12:30 AM on February 7, 2017: member

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).
gmaxwell approved
gmaxwell commented at 4:44 PM on February 9, 2017: contributor

ACK
dcousens approved
theuni commented at 2:13 AM on February 10, 2017: member

#9695 is a superset of this, and those who have acked here have also acked there. Closing in favor.
theuni closed this on Feb 10, 2017
dcousens commented at 3:44 AM on February 10, 2017: contributor

@theuni you just self-referenced this issue?

Did you mean #9708?
theuni commented at 3:57 AM on February 10, 2017: member

@dcousens haha, let's say I was trying to bring github down with a recursive link attack :p

Busted c/p. Yes, 9708. Thanks.
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

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