From 1e66c0d3eefd66fee1ff88dda1ec9d565366ac8f Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Tue, 7 Jul 2026 17:32:25 -0500 Subject: [PATCH] hostmap: unlink a multi-vpnAddr hostinfo from the shared chain exactly once on delete (#1788) --- e2e/handshakes_test.go | 75 ++++++++++++++++++++++++++++++ hostmap.go | 52 ++++++++------------- hostmap_test.go | 101 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 33 deletions(-) diff --git a/e2e/handshakes_test.go b/e2e/handshakes_test.go index d0b9543c..d580eb21 100644 --- a/e2e/handshakes_test.go +++ b/e2e/handshakes_test.go @@ -1535,3 +1535,78 @@ func TestGoodHandshakeUnsafeDest(t *testing.T) { myControl.Stop() theirControl.Stop() } + +func TestMultiVpnAddrDeletePrimaryKeepsSecondAddr(t *testing.T) { + t.Parallel() + // Regression for the hostmap multi-vpnAddr delete bug. A dual-stack (v4+v6) V2-cert peer that + // handshakes twice at once ends up with two hostinfos linked in the shared next/prev chain, with the + // primary owning both addresses. Deleting that primary (e.g. connection manager dropping it, a + // CloseTunnel, a collision) must promote the surviving sibling for EVERY address. The pre-fix code + // unlinked the chain once per address, so it promoted the sibling for the first address and orphaned + // the second: the peer stayed reachable at its v4 addr but not its v6 addr despite a live tunnel. + + ca, _, caKey, _ := cert_test.NewTestCaCert(cert.Version2, cert.Curve_CURVE25519, time.Now(), time.Now().Add(10*time.Minute), nil, nil, []string{}) + myControl, myVpnIpNet, myUdpAddr, _ := newSimpleServer(cert.Version2, ca, caKey, "me ", "10.128.0.1/24,fd00::1/64", nil) + theirControl, theirVpnIpNet, theirUdpAddr, _ := newSimpleServer(cert.Version2, ca, caKey, "them", "10.128.0.2/24,fd00::2/64", nil) + + // This bug only exists for peers carrying more than one vpn address + require.Len(t, theirVpnIpNet, 2) + theirV4 := theirVpnIpNet[0].Addr() + theirV6 := theirVpnIpNet[1].Addr() + + // Put their info in our lighthouse and vice versa + myControl.InjectLightHouseAddr(theirV4, theirUdpAddr) + theirControl.InjectLightHouseAddr(myVpnIpNet[0].Addr(), myUdpAddr) + + // Build a router so we don't have to reason who gets which packet + r := router.NewR(t, myControl, theirControl) + defer r.RenderFlow() + + myControl.Start() + theirControl.Start() + + // Race a handshake so both of us build a hostinfo for the other, leaving my hostmap with a single + // host (them) backed by two linked hostinfos, just like TestStage1Race. + myControl.InjectTunPacket(BuildTunUDPPacket(theirV4, 80, myVpnIpNet[0].Addr(), 80, []byte("Hi from me"))) + theirControl.InjectTunPacket(BuildTunUDPPacket(myVpnIpNet[0].Addr(), 80, theirV4, 80, []byte("Hi from them"))) + + myHsForThem := myControl.GetFromUDP(true) + theirHsForMe := theirControl.GetFromUDP(true) + + r.InjectUDPPacket(theirControl, myControl, theirHsForMe) + r.InjectUDPPacket(myControl, theirControl, myHsForThem) + + r.RouteForAllUntilTxTun(theirControl) + r.RouteForAllUntilTxTun(myControl) + + r.RenderHostmaps("Racing hostmaps", myControl, theirControl) + + // Two hostinfos for them means the shared next/prev chain has a sibling to promote. The Hosts map has + // one entry per vpn address (two, for dual stack), so the index count is what tells us there are two + // hostinfos. + require.Len(t, myControl.ListHostmapIndexes(false), 2) + + // The primary owns both of their addresses + primaryV4 := myControl.GetHostInfoByVpnAddr(theirV4, false) + primaryV6 := myControl.GetHostInfoByVpnAddr(theirV6, false) + require.NotNil(t, primaryV4) + require.NotNil(t, primaryV6) + require.Equal(t, primaryV4.LocalIndex, primaryV6.LocalIndex, "both addrs should point at the same primary") + + // Delete the primary tunnel. localOnly so we don't perturb their side, we only care about my hostmap. + require.True(t, myControl.CloseTunnel(theirV4, true)) + + // The surviving sibling must still serve BOTH addresses. + survivorV4 := myControl.GetHostInfoByVpnAddr(theirV4, false) + survivorV6 := myControl.GetHostInfoByVpnAddr(theirV6, false) + require.NotNil(t, survivorV4, "v4 addr should still resolve to the surviving tunnel") + // Pre-fix this is nil: the second address was orphaned when the primary was deleted. + require.NotNil(t, survivorV6, "v6 addr was orphaned after deleting the primary (multi-vpnAddr delete bug)") + assert.Equal(t, survivorV4.LocalIndex, survivorV6.LocalIndex, "both addrs should promote to the same survivor") + assert.NotEqual(t, primaryV4.LocalIndex, survivorV4.LocalIndex, "a different hostinfo should now be primary") + + r.RenderHostmaps("Final hostmaps", myControl, theirControl) + + myControl.Stop() + theirControl.Stop() +} diff --git a/hostmap.go b/hostmap.go index e7dd17a0..e7399034 100644 --- a/hostmap.go +++ b/hostmap.go @@ -438,43 +438,29 @@ func (hm *HostMap) unlockedMakePrimary(hostinfo *HostInfo) { } func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) { + isLastHostinfo := hostinfo.next == nil && hostinfo.prev == nil + for _, addr := range hostinfo.vpnAddrs { - h := hm.Hosts[addr] - for h != nil { - if h == hostinfo { - hm.unlockedInnerDeleteHostInfo(h, addr) - } - h = h.next + if hm.Hosts[addr] != hostinfo { + continue + } + if hostinfo.next != nil { + // Promote the next hostinfo in the shared chain to primary for this address + hm.Hosts[addr] = hostinfo.next + } else { + delete(hm.Hosts, addr) } } -} + if len(hm.Hosts) == 0 { + hm.Hosts = map[netip.Addr]*HostInfo{} + } -func (hm *HostMap) unlockedInnerDeleteHostInfo(hostinfo *HostInfo, addr netip.Addr) { - primary, ok := hm.Hosts[addr] - isLastHostinfo := hostinfo.next == nil && hostinfo.prev == nil - if ok && primary == hostinfo { - // The vpn addr pointer points to the same hostinfo as the local index id, we can remove it - delete(hm.Hosts, addr) - if len(hm.Hosts) == 0 { - hm.Hosts = map[netip.Addr]*HostInfo{} - } - - if hostinfo.next != nil { - // We had more than 1 hostinfo at this vpn addr, promote the next in the list to primary - hm.Hosts[addr] = hostinfo.next - // It is primary, there is no previous hostinfo now - hostinfo.next.prev = nil - } - - } else { - // Relink if we were in the middle of multiple hostinfos for this vpn addr - if hostinfo.prev != nil { - hostinfo.prev.next = hostinfo.next - } - - if hostinfo.next != nil { - hostinfo.next.prev = hostinfo.prev - } + // Splice this hostinfo out of the shared chain exactly once + if hostinfo.prev != nil { + hostinfo.prev.next = hostinfo.next + } + if hostinfo.next != nil { + hostinfo.next.prev = hostinfo.prev } hostinfo.next = nil diff --git a/hostmap_test.go b/hostmap_test.go index 2bd7bd43..156444a3 100644 --- a/hostmap_test.go +++ b/hostmap_test.go @@ -194,6 +194,107 @@ func TestHostMap_DeleteHostInfo(t *testing.T) { assert.Nil(t, prim) } +// TestHostMap_DeleteHostInfo_MultipleVpnAddrs exercises the case where a hostinfo carries more than one +// vpnAddr and shares its next/prev chain with a live sibling. Deleting the head must not corrupt the +// sibling: every address the sibling owns has to keep pointing at it. The pre-fix code unlinked the shared +// chain once per vpnAddr, so on the first address it nil'd next/prev, and on the second address the node +// looked already-detached: it dropped the map entry instead of promoting the sibling (and tripped the +// isLastHostinfo relay teardown). See unlockedDeleteHostInfo. +func TestHostMap_DeleteHostInfo_MultipleVpnAddrs(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + + f := &Interface{} + + a := netip.MustParseAddr("0.0.0.1") + b := netip.MustParseAddr("0.0.0.2") + + // Two tunnels for the same peer, each reachable at both a and b. + other := &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: 1} + head := &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: 2} + + hm.unlockedAddHostInfo(other, f) + hm.unlockedAddHostInfo(head, f) + + // head is primary for both addresses, other is next in the shared chain + assert.Equal(t, head.localIndexId, hm.QueryVpnAddr(a).localIndexId) + assert.Equal(t, head.localIndexId, hm.QueryVpnAddr(b).localIndexId) + assert.Equal(t, other.localIndexId, head.next.localIndexId) + assert.Equal(t, head.localIndexId, other.prev.localIndexId) + + // Delete the head. other is still live, so it must become primary for BOTH addresses. + hm.DeleteHostInfo(head) + + // Pre-fix: QueryVpnAddr(b) came back nil here because the second address was deleted rather than + // promoted, leaving other unreachable at b. + require.NotNil(t, hm.QueryVpnAddr(a)) + require.NotNil(t, hm.QueryVpnAddr(b)) + assert.Equal(t, other.localIndexId, hm.QueryVpnAddr(a).localIndexId) + assert.Equal(t, other.localIndexId, hm.QueryVpnAddr(b).localIndexId) + + // other is now the only hostinfo in the chain + assert.Nil(t, other.prev) + assert.Nil(t, other.next) + + // head is fully detached + assert.Nil(t, head.prev) + assert.Nil(t, head.next) + assert.Nil(t, hm.QueryIndex(head.localIndexId)) +} + +// TestHostMap_MaxHostInfosPerVpnIp_MultipleVpnAddrs verifies the MaxHostInfosPerVpnIp overflow prune +// (unlockedInnerAddHostInfo calls unlockedDeleteHostInfo on the oldest node once the chain is too long) +// still behaves when hostinfos carry more than one vpnAddr. The pruned node is always the tail, so it is +// primary for none of the addresses, and both address chains must stay consistent afterwards. +func TestHostMap_MaxHostInfosPerVpnIp_MultipleVpnAddrs(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + + f := &Interface{} + + a := netip.MustParseAddr("0.0.0.1") + b := netip.MustParseAddr("0.0.0.2") + + // Add one more than the cap, newest last so it becomes head. Every hostinfo owns both a and b. + hostinfos := make([]*HostInfo, 0, MaxHostInfosPerVpnIp+1) + for i := 0; i <= MaxHostInfosPerVpnIp; i++ { + hostinfos = append(hostinfos, &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: uint32(i + 1)}) + } + // Add oldest first (highest index in our slice) so the very first one added is the overflow victim. + for i := len(hostinfos) - 1; i >= 0; i-- { + hm.unlockedAddHostInfo(hostinfos[i], f) + } + + oldest := hostinfos[len(hostinfos)-1] + + // The oldest hostinfo should have been pruned and fully detached + assert.Nil(t, oldest.next) + assert.Nil(t, oldest.prev) + assert.Nil(t, hm.QueryIndex(oldest.localIndexId)) + + // Both addresses resolve to the same head, and that head is one of the survivors (not the pruned one) + primA := hm.QueryVpnAddr(a) + primB := hm.QueryVpnAddr(b) + require.NotNil(t, primA) + require.NotNil(t, primB) + assert.Equal(t, primA.localIndexId, primB.localIndexId) + assert.NotEqual(t, oldest.localIndexId, primA.localIndexId) + + // Walk the shared chain: exactly MaxHostInfosPerVpnIp survivors, no cycles, oldest absent + seen := map[uint32]struct{}{} + for h := primA; h != nil; h = h.next { + _, dup := seen[h.localIndexId] + require.False(t, dup, "cycle detected in hostinfo chain") + seen[h.localIndexId] = struct{}{} + if h.next != nil { + assert.Equal(t, h.localIndexId, h.next.prev.localIndexId, "prev pointer must mirror next") + } + } + assert.Len(t, seen, MaxHostInfosPerVpnIp) + _, prunedStillPresent := seen[oldest.localIndexId] + assert.False(t, prunedStillPresent) +} + func TestHostMap_reload(t *testing.T) { l := test.NewLogger() c := config.NewC(test.NewLogger())