Clean up all known races/platform-specific UB at the time PR was opened #9708
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Concept ACK. Lightly tested ack. Getting these cleaned up makes it much easier to use tools that catch serious bugs. |
dcousens
approved these changes
Feb 8, 2017
theuni
reviewed
Feb 9, 2017
src/net.cpp
Outdated
|
|
||
| void CNode::MaybeSetAddrName(const std::string& addrNameIn) { | ||
| LOCK(cs_addrName); | ||
| if (addrName == "") { |
TheBlueMatt
force-pushed
the
2017-02-fix-copystats-races
branch
from
74b46ed to
a51ecf7
Compare
|
I added clang annotations for everything in net (I think) that's guarded by a mutex, and built with -Wthread-safety: theuni@df50d4f This reveals that nearly all is good, with these exceptions: theuni@d185ca0 The only one that we really need to worry about for 0.14 is the cs_filter locking. However, because cs_filter is locked from net_processing with a wide scope in a few places, I'm not confident that the So, options are:
I'm fine with any of the above. |
TheBlueMatt
changed the title
|
Heh, title change comes from IRC discussion. The race mentioned above is not new, and not likely to cause issues. It's not worth holding up 0.14 for another set of ACKs or another PR to review. If anyone strongly disagrees I'll PR a fix, but I'm ok with leaving it alone. ACK a51ecf7ce00096bf607d15227ff1e1e39a3f6803 |
|
What should hold up the release are critical issues, e.g. those that can (or do) cause crashes, and corruption. Fixing every little thing static and dynamic analyzers can complain about is not part of that, and can be done later. utACK a51ecf7 |
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
TheBlueMatt
force-pushed
the
2017-02-fix-copystats-races
branch
from
a51ecf7 to
db2dc7a
Compare
|
Rebased to fix a trivial merge conflict. |
|
re-ACK db2dc7a |
|
ACK db2dc7a. Running a node with master + this PR, with many connections, compiled with -fsanitize=thread -fsanitize=undefined. Only these cases are detected:
|
sipa
added a commit
that referenced
this pull request
Feb 11, 2017
…e 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)
LarryRuane
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
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 zcash: cherry picked from commit 321d0fc zcash: bitcoin/bitcoin#9708
LarryRuane
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit 644f123 zcash: bitcoin/bitcoin#9708
LarryRuane
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit ae683c1 zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit 512731b zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit 96f42d8 zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit 0f31872 zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit 22b4966 zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit d8f2b8a zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit 036073b zcash: bitcoin/bitcoin#9708
str4d
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 1, 2021
zcash: cherry picked from commit db2dc7a zcash: bitcoin/bitcoin#9708
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Apr 2, 2021
Bitcoin 0.14 locking PRs These are locking changes from upstream (bitcoin core) release 0.14, oldest to newest (when merged to the master branch). Each commit also includes a reference both to the PR and the upstream commit. - bitcoin/bitcoin#8472 - bitcoin/bitcoin#8606 - Excludes a lock move because we don't have bitcoin/bitcoin#7840 which this move was partially-reverting. - bitcoin/bitcoin#9230 - Only first commit (we don't have `LogTimestampStr` anymore). - bitcoin/bitcoin#9243 - Only the sixth commit, excluding `IsArgSet` locking (we haven't pulled that function in yet). - bitcoin/bitcoin#9626 - The cherry-picked commit does not match the upstream at all, but the resulting lock is useful. - bitcoin/bitcoin#9679 - bitcoin/bitcoin#9227 - bitcoin/bitcoin#9698 - Excludes change to `CConnman::PushMessage` in second commit (which we don't have yet). - bitcoin/bitcoin#9708 - bitcoin/bitcoin#9771
LarryRuane
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Apr 26, 2021
zcash: cherry picked from commit 512731b zcash: bitcoin/bitcoin#9708
LarryRuane
pushed a commit
to LarryRuane/zcash
that referenced
this pull request
Jun 1, 2021
zcash: cherry picked from commit 512731b zcash: bitcoin/bitcoin#9708
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because we (finally) have C++11, there is no excuse for using ints/flags/anything concurrently.
#9695 (which this is based on) got most of them, but the last two (addrName and addrLocal) were left out because they might need more discussion. Here I opted for giving them a wrapper which does locked access, though that does mean creating copies of them during use.