diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 580df92b..fe120a62 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -735,19 +735,21 @@ func (node *Node) ApplyPeerChange(change *tailcfg.PeerChange) { // This might technically not be useful as we replace // the whole hostinfo blob when it has changed. if change.DERPRegion != 0 { - if node.Hostinfo == nil { - node.Hostinfo = &tailcfg.Hostinfo{ - NetInfo: &tailcfg.NetInfo{ - PreferredDERP: change.DERPRegion, - }, - } - } else if node.Hostinfo.NetInfo == nil { - node.Hostinfo.NetInfo = &tailcfg.NetInfo{ - PreferredDERP: change.DERPRegion, - } - } else { - node.Hostinfo.NetInfo.PreferredDERP = change.DERPRegion + // [NodeStore] publishes snapshots that share the *Hostinfo / + // *NetInfo pointers, so writing PreferredDERP in place would race + // readers of an already-published snapshot. Clone to fresh pointers + // and assign, leaving the previous snapshot untouched. + hi := node.Hostinfo.Clone() + if hi == nil { + hi = &tailcfg.Hostinfo{} } + + if hi.NetInfo == nil { + hi.NetInfo = &tailcfg.NetInfo{} + } + + hi.NetInfo.PreferredDERP = change.DERPRegion + node.Hostinfo = hi } node.LastSeen = change.LastSeen diff --git a/hscontrol/types/node_peerchange_test.go b/hscontrol/types/node_peerchange_test.go new file mode 100644 index 00000000..d344d888 --- /dev/null +++ b/hscontrol/types/node_peerchange_test.go @@ -0,0 +1,61 @@ +package types + +import ( + "testing" + + "tailscale.com/tailcfg" +) + +// TestApplyPeerChangeDERPDoesNotMutateSharedHostinfo guards the NodeStore +// copy-on-write invariant: published snapshots share the *tailcfg.Hostinfo / +// *tailcfg.NetInfo pointers, so ApplyPeerChange must never write a DERP region +// through them in place. It must produce fresh pointers, leaving any +// previously-shared Hostinfo untouched. +func TestApplyPeerChangeDERPDoesNotMutateSharedHostinfo(t *testing.T) { + const newRegion = 2 + + t.Run("existing NetInfo", func(t *testing.T) { + shared := &tailcfg.Hostinfo{ + Hostname: "n", + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + } + node := &Node{Hostinfo: shared} + + node.ApplyPeerChange(&tailcfg.PeerChange{DERPRegion: newRegion}) + + if got := node.Hostinfo.NetInfo.PreferredDERP; got != newRegion { + t.Fatalf("node PreferredDERP = %d, want %d", got, newRegion) + } + + if got := shared.NetInfo.PreferredDERP; got != 1 { + t.Errorf("shared NetInfo mutated in place: PreferredDERP = %d, want 1", got) + } + + if node.Hostinfo == shared { + t.Error("node.Hostinfo still aliases the shared Hostinfo pointer") + } + + if node.Hostinfo.NetInfo == shared.NetInfo { + t.Error("node.Hostinfo.NetInfo still aliases the shared NetInfo pointer") + } + }) + + t.Run("nil NetInfo", func(t *testing.T) { + shared := &tailcfg.Hostinfo{Hostname: "n"} + node := &Node{Hostinfo: shared} + + node.ApplyPeerChange(&tailcfg.PeerChange{DERPRegion: newRegion}) + + if got := node.Hostinfo.NetInfo.PreferredDERP; got != newRegion { + t.Fatalf("node PreferredDERP = %d, want %d", got, newRegion) + } + + if shared.NetInfo != nil { + t.Errorf("shared Hostinfo gained a NetInfo in place: %+v", shared.NetInfo) + } + + if node.Hostinfo == shared { + t.Error("node.Hostinfo still aliases the shared Hostinfo pointer") + } + }) +}