From 25f12e8a8fdc0b56d84614b7a33ec5af4c877af6 Mon Sep 17 00:00:00 2001 From: Jonathan Chappelow Date: Wed, 13 Oct 2021 18:19:57 -0500 Subject: [PATCH] addrmgr: make safe accessors for addresses 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. --- addrmgr/addrmanager.go | 45 +++++++++++++++++++++++++++++++++++++++--- server.go | 14 ++++++------- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/addrmgr/addrmanager.go b/addrmgr/addrmanager.go index fa8f27bcae..1cfbbc5ca4 100644 --- a/addrmgr/addrmanager.go +++ b/addrmgr/addrmanager.go @@ -176,7 +176,7 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) { // TODO: only update addresses periodically. // Update the last seen time and services. // note that to prevent causing excess garbage on getaddr - // messages the netaddresses in addrmaanger are *immutable*, + // messages the netaddresses in addrmanager are *immutable*, // if we need to change them then we replace the pointer with a // new copy so that we don't have to copy every na for getaddr. if netAddr.Timestamp.After(ka.na.Timestamp) || @@ -186,7 +186,7 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) { naCopy := *ka.na naCopy.Timestamp = netAddr.Timestamp naCopy.AddService(netAddr.Services) - ka.na = &naCopy + ka.na = &naCopy // warning: na field of KnownAddress is mutable } // If already in tried, we have nothing to do here. @@ -773,11 +773,50 @@ func NetAddressKey(na *wire.NetAddress) string { // random one from the possible addresses with preference given to ones that // have not been used recently and should not pick 'close' addresses // consecutively. +// +// NOTE: The KnownAddress returned by GetAddress is not safely usable while the +// AddrManager is in use by another goroutine since the fields of KnownAddress +// are mutable. Is this method practical outside of tests that use a toy +// AddrManager? func (a *AddrManager) GetAddress() *KnownAddress { - // Protect concurrent access. a.mtx.Lock() defer a.mtx.Unlock() + return a.getAddress() +} + +// GetAddressCopy is like GetAddress, except that this returns a copy of a +// stored KnownAddress, so is safe for concurrent access. +func (a *AddrManager) GetAddressCopy() *KnownAddress { + a.mtx.Lock() + defer a.mtx.Unlock() + ka := a.getAddress() + if ka == nil { + return nil + } + // While the NetAddress in ka.na is immutable, the na field of KnownAddress + // is not, so we access it under the address manager's lock. See + // updateAddress, SetServices, Connected, and Good, where various + // KnownAddress fields are modified that are also accessible by KnownAddress + // methods without a synchronization mechanism. + kaCopy := *ka + return &kaCopy +} + +// GetNetAddress returns a single address that should be routable and its last +// connection attempt time. It picks a random one from the possible addresses +// with preference given to ones that have not been used recently and should not +// pick 'close' addresses consecutively. +func (a *AddrManager) GetNetAddress() (*wire.NetAddress, time.Time) { + a.mtx.Lock() + defer a.mtx.Unlock() + ka := a.getAddress() + if ka == nil { + return nil, time.Time{} + } + return ka.na, ka.lastattempt +} +func (a *AddrManager) getAddress() *KnownAddress { if a.numAddresses() == 0 { return nil } diff --git a/server.go b/server.go index 746c48dde3..a2bb9ea38b 100644 --- a/server.go +++ b/server.go @@ -2843,8 +2843,8 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string, if !cfg.SimNet && len(cfg.ConnectPeers) == 0 { newAddressFunc = func() (net.Addr, error) { for tries := 0; tries < 100; tries++ { - addr := s.addrManager.GetAddress() - if addr == nil { + netAddr, lastAttempt := s.addrManager.GetNetAddress() + if netAddr == nil { break } @@ -2854,27 +2854,27 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string, // in the same group so that we are not connecting // to the same network segment at the expense of // others. - key := addrmgr.GroupKey(addr.NetAddress()) + key := addrmgr.GroupKey(netAddr) if s.OutboundGroupCount(key) != 0 { continue } // only allow recent nodes (10mins) after we failed 30 // times - if tries < 30 && time.Since(addr.LastAttempt()) < 10*time.Minute { + if tries < 30 && time.Since(lastAttempt) < 10*time.Minute { continue } // allow nondefault ports after 50 failed tries. - if tries < 50 && fmt.Sprintf("%d", addr.NetAddress().Port) != + if tries < 50 && fmt.Sprintf("%d", netAddr.Port) != activeNetParams.DefaultPort { continue } // Mark an attempt for the valid address. - s.addrManager.Attempt(addr.NetAddress()) + s.addrManager.Attempt(netAddr) - addrString := addrmgr.NetAddressKey(addr.NetAddress()) + addrString := addrmgr.NetAddressKey(netAddr) return addrStringToNetAddr(addrString) }