hostmap: unlink a multi-vpnAddr hostinfo from the shared chain exactly once on delete (#1788)

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()
+}

hostmap.go

@@ -438,44 +438,30 @@ func (hm *HostMap) unlockedMakePrimary(hostinfo *HostInfo) {
 }
 
 func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) {
-	for _, addr := range hostinfo.vpnAddrs {
-		h := hm.Hosts[addr]
-		for h != nil {
-			if h == hostinfo {
-				hm.unlockedInnerDeleteHostInfo(h, addr)
-			}
-			h = h.next
-		}
-	}
-}
-
-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
+	for _, addr := range hostinfo.vpnAddrs {
+		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{}
 	}
 
-		if hostinfo.next != nil {
+	// Splice this hostinfo out of the shared chain exactly once
-			// 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
 	}
-	}
 
 	hostinfo.next = nil
 	hostinfo.prev = nil

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())