tstest/integration/testcontrol: fix serveMap read-modify-write race
serveMap cloned s.nodes[nk], mutated the clone outside the mutex, then wrote it back via updateNodeLocked. A concurrent UpdateNode, SetNodeCapMap, or other writer landing between the clone and the writeback would be silently clobbered. Mutate the live node under the mutex instead. Surfaces in tsnet's TestListenService as a flaky ErrUntaggedServiceHost panic: the test calls control.UpdateNode to attach a tag, a concurrent updateRoutine map request from the host races, and the host's next netmap arrives with Tags=[]. Updates #19822 Change-Id: I6c5ebd5e5bf79a40316f53f627157230773cb469 Signed-off-by: James Tucker <james@tailscale.com>
This commit is contained in:
committed by
James Tucker
parent
61277e3ad4
commit
36c52ef383
@@ -1215,19 +1215,31 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi
|
|||||||
var peersToUpdate []tailcfg.NodeID
|
var peersToUpdate []tailcfg.NodeID
|
||||||
if !req.ReadOnly && !streamingNonUpdate {
|
if !req.ReadOnly && !streamingNonUpdate {
|
||||||
endpoints := filterInvalidIPv6Endpoints(req.Endpoints)
|
endpoints := filterInvalidIPv6Endpoints(req.Endpoints)
|
||||||
node.Endpoints = endpoints
|
var hi tailcfg.HostinfoView
|
||||||
node.DiscoKey = req.DiscoKey
|
var newDERP int
|
||||||
node.Cap = req.Version
|
|
||||||
if req.Hostinfo != nil {
|
if req.Hostinfo != nil {
|
||||||
node.Hostinfo = req.Hostinfo.View()
|
hi = req.Hostinfo.View()
|
||||||
if ni := node.Hostinfo.NetInfo(); ni.Valid() {
|
if ni := hi.NetInfo(); ni.Valid() {
|
||||||
if ni.PreferredDERP() != 0 {
|
newDERP = ni.PreferredDERP()
|
||||||
node.HomeDERP = ni.PreferredDERP()
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Mutate the live node under the mutex; writing back the clone
|
||||||
|
// obtained above would clobber any concurrent writer's changes
|
||||||
|
// to other fields (e.g. UpdateNode, SetNodeCapMap).
|
||||||
s.mu.Lock()
|
s.mu.Lock()
|
||||||
peersToUpdate = s.updateNodeLocked(node)
|
live := s.nodes[req.NodeKey]
|
||||||
|
if live != nil {
|
||||||
|
live.Endpoints = endpoints
|
||||||
|
live.DiscoKey = req.DiscoKey
|
||||||
|
live.Cap = req.Version
|
||||||
|
if hi.Valid() {
|
||||||
|
live.Hostinfo = hi
|
||||||
|
if newDERP != 0 {
|
||||||
|
live.HomeDERP = newDERP
|
||||||
|
}
|
||||||
|
}
|
||||||
|
peersToUpdate = s.nodeIDsLocked(live.ID)
|
||||||
|
}
|
||||||
s.mu.Unlock()
|
s.mu.Unlock()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user