evict old paths when the client probes a new path (#5034)

* store paths in a slice, not a map No functional change expected. * save the timestamp of the last received packet for a path * evict old paths * reduce the path timeout to 5s
2025-04-19 10:10:54 +08:00
parent 7cffd500f1
commit cda52a1e36
3 changed files with 97 additions and 32 deletions
--- a/connection.go
+++ b/connection.go
@@ -1075,7 +1075,7 @@ func (s *connection) handleShortHeaderPacket(p receivedPacket, isCoalesced bool)
 			s.logger,
 		)
 	}
-	destConnID, frames, shouldSwitchPath := s.pathManager.HandlePacket(p.remoteAddr, pathChallenge, isNonProbing)
+	destConnID, frames, shouldSwitchPath := s.pathManager.HandlePacket(p.remoteAddr, p.rcvTime, pathChallenge, isNonProbing)
 	if len(frames) > 0 {
 		probe, buf, err := s.packer.PackPathProbePacket(destConnID, frames, s.version)
 		if err != nil {
--- a/path_manager.go
+++ b/path_manager.go
@@ -3,6 +3,8 @@ package quic
 import (
 	"crypto/rand"
 	"net"
+	"slices"
+	"time"

 	"github.com/quic-go/quic-go/internal/ackhandler"
 	"github.com/quic-go/quic-go/internal/protocol"
@@ -14,10 +16,21 @@ type pathID int64

 const invalidPathID pathID = -1

+// Maximum number of paths to keep track of.
+// If the peer probes another path (before the pathTimeout of an existing path expires),
+// this probing attempt is ignored.
 const maxPaths = 3

+// If no packet is received for a path for pathTimeout,
+// the path can be evicted when the peer probes another path.
+// This prevents an attacker from churning through paths by duplicating packets and
+// sending them with spoofed source addresses.
+const pathTimeout = 5 * time.Second
+
 type path struct {
+	id             pathID
 	addr           net.Addr
+	lastPacketTime time.Time
 	pathChallenge  [8]byte
 	validated      bool
 	rcvdNonProbing bool
@@ -25,7 +38,8 @@ type path struct {

 type pathManager struct {
 	nextPathID pathID
-	paths      map[pathID]*path
+	// ordered by lastPacketTime, with the most recently used path at the end
+	paths []*path

 	getConnID    func(pathID) (_ protocol.ConnectionID, ok bool)
 	retireConnID func(pathID)
@@ -39,7 +53,7 @@ func newPathManager(
 	logger utils.Logger,
 ) *pathManager {
 	return &pathManager{
-		paths:        make(map[pathID]*path),
+		paths:        make([]*path, 0, maxPaths+1),
 		getConnID:    getConnID,
 		retireConnID: retireConnID,
 		logger:       logger,
@@ -50,15 +64,15 @@ func newPathManager(
 // May return nil.
 func (pm *pathManager) HandlePacket(
 	remoteAddr net.Addr,
+	t time.Time,
 	pathChallenge *wire.PathChallengeFrame, // may be nil if the packet didn't contain a PATH_CHALLENGE
 	isNonProbing bool,
 ) (_ protocol.ConnectionID, _ []ackhandler.Frame, shouldSwitch bool) {
 	var p *path
-	pathID := pm.nextPathID
-	for id, path := range pm.paths {
+	for i, path := range pm.paths {
 		if addrsEqual(path.addr, remoteAddr) {
 			p = path
-			pathID = id
+			p.lastPacketTime = t
 			// already sent a PATH_CHALLENGE for this path
 			if isNonProbing {
 				path.rcvdNonProbing = true
@@ -67,6 +81,11 @@ func (pm *pathManager) HandlePacket(
 				pm.logger.Debugf("received packet for path %s that was already probed, validated: %t", remoteAddr, path.validated)
 			}
 			shouldSwitch = path.validated && path.rcvdNonProbing
+			if i != len(pm.paths)-1 {
+				// move the path to the end of the list
+				pm.paths = slices.Delete(pm.paths, i, i+1)
+				pm.paths = append(pm.paths, p)
+			}
 			if pathChallenge == nil {
 				return protocol.ConnectionID{}, nil, shouldSwitch
 			}
@@ -74,10 +93,22 @@ func (pm *pathManager) HandlePacket(
 	}

 	if len(pm.paths) >= maxPaths {
-		if pm.logger.Debug() {
-			pm.logger.Debugf("received packet for previously unseen path %s, but already have %d paths", remoteAddr, len(pm.paths))
+		if pm.paths[0].lastPacketTime.Add(pathTimeout).After(t) {
+			if pm.logger.Debug() {
+				pm.logger.Debugf("received packet for previously unseen path %s, but already have %d paths", remoteAddr, len(pm.paths))
+			}
+			return protocol.ConnectionID{}, nil, shouldSwitch
 		}
-		return protocol.ConnectionID{}, nil, shouldSwitch
+		// evict the oldest path, if the last packet was received more than pathTimeout ago
+		pm.retireConnID(pm.paths[0].id)
+		pm.paths = pm.paths[1:]
+	}
+
+	var pathID pathID
+	if p != nil {
+		pathID = p.id
+	} else {
+		pathID = pm.nextPathID
 	}

 	// previously unseen path, initiate path validation by sending a PATH_CHALLENGE
@@ -92,16 +123,18 @@ func (pm *pathManager) HandlePacket(
 		var pathChallengeData [8]byte
 		rand.Read(pathChallengeData[:])
 		p = &path{
+			id:             pm.nextPathID,
 			addr:           remoteAddr,
+			lastPacketTime: t,
 			rcvdNonProbing: isNonProbing,
 			pathChallenge:  pathChallengeData,
 		}
+		pm.nextPathID++
+		pm.paths = append(pm.paths, p)
 		frames = append(frames, ackhandler.Frame{
 			Frame:   &wire.PathChallengeFrame{Data: p.pathChallenge},
 			Handler: (*pathManagerAckHandler)(pm),
 		})
-		pm.paths[pm.nextPathID] = p
-		pm.nextPathID++
 		pm.logger.Debugf("enqueueing PATH_CHALLENGE for new path %s", remoteAddr)
 	}
 	if pathChallenge != nil {
@@ -127,14 +160,15 @@ func (pm *pathManager) HandlePathResponseFrame(f *wire.PathResponseFrame) {
 // SwitchToPath is called when the connection switches to a new path
 func (pm *pathManager) SwitchToPath(addr net.Addr) {
 	// retire all other paths
-	for id := range pm.paths {
-		if addrsEqual(pm.paths[id].addr, addr) {
-			pm.logger.Debugf("switching to path %d (%s)", id, addr)
+	for _, path := range pm.paths {
+		if addrsEqual(path.addr, addr) {
+			pm.logger.Debugf("switching to path %d (%s)", path.id, addr)
 			continue
 		}
-		pm.retireConnID(id)
+		pm.retireConnID(path.id)
 	}
 	clear(pm.paths)
+	pm.paths = pm.paths[:0]
 }

 type pathManagerAckHandler pathManager
@@ -147,10 +181,10 @@ func (pm *pathManagerAckHandler) OnAcked(f wire.Frame) {}
 func (pm *pathManagerAckHandler) OnLost(f wire.Frame) {
 	// TODO: retransmit the packet the first time it is lost
 	pc := f.(*wire.PathChallengeFrame)
-	for id, path := range pm.paths {
+	for i, path := range pm.paths {
 		if path.pathChallenge == pc.Data {
-			delete(pm.paths, id)
-			pm.retireConnID(id)
+			pm.paths = slices.Delete(pm.paths, i, i+1)
+			pm.retireConnID(path.id)
 			break
 		}
 	}
--- a/path_manager_test.go
+++ b/path_manager_test.go
@@ -4,6 +4,7 @@ import (
 	"crypto/rand"
 	"net"
 	"testing"
+	"time"

 	"github.com/quic-go/quic-go/internal/ackhandler"
 	"github.com/quic-go/quic-go/internal/protocol"
@@ -28,8 +29,10 @@ func TestPathManagerIntentionalMigration(t *testing.T) {
 		func(id pathID) { retiredConnIDs = append(retiredConnIDs, connIDs[id]) },
 		utils.DefaultLogger,
 	)
+	now := time.Now()
 	connID, frames, shouldSwitch := pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		&wire.PathChallengeFrame{Data: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}},
 		false,
 	)
@@ -46,6 +49,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {
 	// receiving another packet for the same path doesn't trigger another PATH_CHALLENGE
 	connID, frames, shouldSwitch = pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		nil,
 		false,
 	)
@@ -55,7 +59,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {

 	// receiving a packet for a different path triggers another PATH_CHALLENGE
 	addr2 := &net.UDPAddr{IP: net.IPv4(5, 6, 7, 8), Port: 1000}
-	connID, frames, shouldSwitch = pm.HandlePacket(addr2, nil, false)
+	connID, frames, shouldSwitch = pm.HandlePacket(addr2, now, nil, false)
 	require.Equal(t, connIDs[1], connID)
 	require.Len(t, frames, 1)
 	require.IsType(t, &wire.PathChallengeFrame{}, frames[0].Frame)
@@ -67,6 +71,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {
 	frames[0].Handler.OnAcked(frames[0].Frame)
 	connID, frames, shouldSwitch = pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		nil,
 		false,
 	)
@@ -76,7 +81,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {

 	// receiving a PATH_RESPONSE for the second path confirms the path
 	pm.HandlePathResponseFrame(&wire.PathResponseFrame{Data: pc2.Data})
-	connID, frames, shouldSwitch = pm.HandlePacket(addr2, nil, false)
+	connID, frames, shouldSwitch = pm.HandlePacket(addr2, now, nil, false)
 	require.Zero(t, connID)
 	require.Empty(t, frames)
 	require.False(t, shouldSwitch) // no non-probing packet received yet
@@ -85,6 +90,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {
 	// confirming the path doesn't remove other paths
 	connID, frames, shouldSwitch = pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		nil,
 		false,
 	)
@@ -95,6 +101,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {
 	// now receive a non-probing packet for the new path
 	connID, frames, shouldSwitch = pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(5, 6, 7, 8), Port: 1000},
+		now,
 		nil,
 		true,
 	)
@@ -106,7 +113,7 @@ func TestPathManagerIntentionalMigration(t *testing.T) {
 	pm.SwitchToPath(&net.UDPAddr{IP: net.IPv4(5, 6, 7, 8), Port: 1000})

 	// switching to the path removes other paths
-	connID, frames, shouldSwitch = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000}, nil, false)
+	connID, frames, shouldSwitch = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000}, now, nil, false)
 	require.Equal(t, connIDs[2], connID)
 	require.NotEmpty(t, frames)
 	require.NotEqual(t, frames[0].Frame.(*wire.PathChallengeFrame).Data, pc1.Data)
@@ -123,9 +130,11 @@ func TestPathManagerMultipleProbes(t *testing.T) {
 		func(id pathID) {},
 		utils.DefaultLogger,
 	)
+	now := time.Now()
 	// first receive a packet without a PATH_CHALLENGE
 	connID, frames, shouldSwitch := pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		nil,
 		false,
 	)
@@ -137,6 +146,7 @@ func TestPathManagerMultipleProbes(t *testing.T) {
 	// now receive a packet on the same path with a PATH_CHALLENGE
 	connID, frames, shouldSwitch = pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		&wire.PathChallengeFrame{Data: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}},
 		false,
 	)
@@ -148,6 +158,7 @@ func TestPathManagerMultipleProbes(t *testing.T) {
 	// now receive an other packet on the same path with a PATH_RESPONSE
 	connID, frames, shouldSwitch = pm.HandlePacket(
 		&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000},
+		now,
 		&wire.PathChallengeFrame{Data: [8]byte{8, 7, 6, 5, 4, 3, 2, 1}},
 		false,
 	)
@@ -171,7 +182,8 @@ func TestPathManagerNATRebinding(t *testing.T) {
 		utils.DefaultLogger,
 	)

-	connID, frames, shouldSwitch := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000}, nil, true)
+	now := time.Now()
+	connID, frames, shouldSwitch := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000}, now, nil, true)
 	require.Equal(t, connIDs[0], connID)
 	require.Len(t, frames, 1)
 	require.IsType(t, &wire.PathChallengeFrame{}, frames[0].Frame)
@@ -182,7 +194,7 @@ func TestPathManagerNATRebinding(t *testing.T) {
 	// receiving a PATH_RESPONSE for the second path confirms the path
 	pm.HandlePathResponseFrame(&wire.PathResponseFrame{Data: pc1.Data})
 	// we now switch to the new path, as soon as the next packet on that path is received
-	connID, frames, shouldSwitch = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000}, nil, false)
+	connID, frames, shouldSwitch = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000}, now, nil, false)
 	require.Zero(t, connID)
 	require.Empty(t, frames)
 	require.True(t, shouldSwitch)
@@ -190,7 +202,7 @@ func TestPathManagerNATRebinding(t *testing.T) {

 func TestPathManagerLimits(t *testing.T) {
 	var connIDs []protocol.ConnectionID
-	for range 2*maxPaths + 1 {
+	for range 2*maxPaths + 2 {
 		b := make([]byte, 8)
 		rand.Read(b)
 		connIDs = append(connIDs, protocol.ParseConnectionID(b))
@@ -202,29 +214,48 @@ func TestPathManagerLimits(t *testing.T) {
 		utils.DefaultLogger,
 	)

+	now := time.Now()
+	firstPathTime := now
+	var firstPathConnID protocol.ConnectionID
+	require.Greater(t, pathTimeout, maxPaths*time.Second)
 	for i := range maxPaths {
-		connID, frames, _ := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000 + i}, nil, true)
+		connID, frames, _ := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000 + i}, now, nil, true)
 		require.NotEmpty(t, frames)
 		require.Equal(t, connIDs[i], connID)
+		if i == 0 {
+			firstPathConnID = connID
+		}
+		now = now.Add(time.Second)
 	}
 	// the maximum number of paths is already being probed
-	connID, frames, _ := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 2000}, nil, true)
+	now = firstPathTime.Add(pathTimeout).Add(-time.Nanosecond)
+	connID, frames, _ := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 2000}, now, nil, true)
 	require.Zero(t, connID)
 	require.Empty(t, frames)

+	// receiving another packet after the pathTimeout of the first path evicts the first path
+	now = firstPathTime.Add(pathTimeout)
+	connIDIndex := maxPaths
+	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000 + maxPaths}, now, nil, true)
+	require.NotEmpty(t, frames)
+	require.Equal(t, connIDs[connIDIndex], connID)
+	require.Equal(t, []protocol.ConnectionID{firstPathConnID}, retiredConnIDs)
+	connIDIndex++
+
 	// switching to a new path frees is up all paths
 	var f1 []ackhandler.Frame
 	pm.SwitchToPath(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1000})
 	for i := range maxPaths {
-		connID, frames, _ := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 3000 + i}, nil, true)
+		connID, frames, _ := pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 3000 + i}, now, nil, true)
 		if i == 0 {
 			f1 = frames
 		}
 		require.NotEmpty(t, frames)
-		require.Equal(t, connIDs[maxPaths+i], connID)
+		require.Equal(t, connIDs[connIDIndex], connID)
+		connIDIndex++
 	}
 	// again, the maximum number of paths is already being probed
-	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 2000}, nil, true)
+	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 2000}, now, nil, true)
 	require.Zero(t, connID)
 	require.Empty(t, frames)

@@ -232,10 +263,10 @@ func TestPathManagerLimits(t *testing.T) {
 	f1[0].Handler.OnLost(f1[0].Frame)

 	// we can open exactly one more path
-	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 4000}, nil, true)
+	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 4000}, now, nil, true)
 	require.NotEmpty(t, frames)
-	require.Equal(t, connIDs[2*maxPaths], connID)
-	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 4001}, nil, true)
+	require.Equal(t, connIDs[connIDIndex], connID)
+	connID, frames, _ = pm.HandlePacket(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 4001}, now, nil, true)
 	require.Zero(t, connID)
 	require.Empty(t, frames)
 }