addrmgr: make safe accessors for addresses #1760
Closed
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
The KnownAddress returned by GetAddress is not safely usable while the AddrManager is in use by another goroutine since the fields of the KnownAddress are mutable. Although AddrManager treats a *wire.NetAddress as immutable, the KnownAddress.na field itself is reassigned. Similarly, the lastattempt field is mutable. To address this API usability issue, this commit makes two new accessor methods: - GetNetAddress returns the *wire.NetAddress and the lastAttempt - GetAddressCopy returns a copy of the *KnownAddress The original GetAddress remains because it is designed to reference the KnownAddress that is still in the AddrManager, allowing operations by the AddrManager such as connects and attempts to be reflected in the initially returned KnownAddress. The blackbox addrmgr tests are are designed around this behavior. However, it is not safe to use GetAddress from another goroutine such as the connmgr's GetNewAddress function (newAddressFunc closure defined in newServer) concurrent with updates to the KnownAddress such as via (*Peer).inHandler, which has callbacks to the AddrManager that may mutate the KnownAddress. An alternate resolution would involve adding a Mutex to a KnownAddress, but this would be a more substantial change, which would likely create unnecessary lock contention and complexity. Returning copies is safer and simpler.
|
I've noted that dcrd chose to add a Mutex to KnownAddress to solve the data race. decred/dcrd@ad33b0e I'd be happy to take that approach as well, but my feeling was that would add a lot of unnecessary locking when the |
This was referenced Oct 18, 2021
Pull Request Test Coverage Report for Build 1349537398
💛 - Coveralls |
|
Closing this in favor of #1763 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This resolves a data race I have observed with both stock btcwallet run in spv mode, and a different app using neutrino's
ChainService. The races are at the end of this PR description. The explanation and resolution are as follows.NOTE: An alternative resolution that adds a mutex to
KnownAddressis proposed in #1763The
KnownAddressreturned by(*AddrManager).GetAddressis not safely usable while theAddrManageris in use by another goroutine since the fields of theKnownAddressare mutable. AlthoughAddrManagertreats a*wire.NetAddressas immutable, theKnownAddress.nafield itself is reassigned. Similarly, thelastattemptfield is mutable.To address this API usability issue, this commit makes two new accessor methods:
GetNetAddressreturns the*wire.NetAddressand thelastAttemptGetAddressCopyreturns a copy of the*KnownAddress. This is not used in the fixednewAddressFunc, but I've created it for any previousGetAddressconsumers that do want a fullKnownAddressfor whatever reason.The original
GetAddressremains because it is designed to reference theKnownAddressthat is still in theAddrManager, allowing operations by theAddrManagersuch as connects and attempts to be reflected in the initially returnedKnownAddress. The blackboxaddrmgrpackage tests are are designed around this behavior.However, it is not safe to use
GetAddressfrom another goroutine such as theconnmgr'sGetNewAddressfunction (thenewAddressFuncclosure defined innewServer) concurrent with updates to theKnownAddresssuch as via(*Peer).inHandler, which has callbacks to theAddrManagerthat may mutate theKnownAddress.An alternate resolution would involve adding a mutex to a
KnownAddress, but this would be a more substantial change, which creates unnecessary lock contention and complexity. Returning copies or just the needed fields is safer and simpler.While the following data races are using neutrino's
ChainService, the root cause traces back to the btcd API issue above. In both cases it comes down to use of theKnownAddressreturned byGetAddressin thenewAddressFuncclosure, concurrent with a write elsewhere.Data race with custom spv wallet app using neutrino.
Data race with stock btcwallet in spv mode (using neutrino). Partially incomplete dump it seems.
I also have updates ready to neutrino and btcwallet that use this change.