diff --git a/derp/derp_server.go b/derp/derp_server.go index 3021241094d27..28db98cc84003 100644 --- a/derp/derp_server.go +++ b/derp/derp_server.go @@ -26,6 +26,7 @@ import ( "net/netip" "os/exec" "runtime" + "slices" "strconv" "strings" "sync" @@ -233,10 +234,14 @@ type dupClientSet struct { // data since. last *sclient - // sendHistory is a log of which members of set have sent - // frames to the derp server, with adjacent duplicates - // removed. When a member of set is removed, the same - // element(s) are removed from sendHistory. + // sendHistory records which members of set have sent frames to + // the DERP server, ordered from least to most recently active. + // Each member appears at most once: recording a member that is + // already present moves it to the end instead of appending a + // duplicate. That keeps the slice bounded by the size of set. + // Without the bound, two connections sharing a key and taking + // turns sending could grow it without limit. When a member of + // set is removed, it is also removed from sendHistory. sendHistory []*sclient } @@ -1211,7 +1216,14 @@ func (s *Server) noteClientActivity(c *sclient) { } } - // Append this client to the list of clients who spoke last. + // Record c as the most recent sender. If c is already in + // sendHistory, remove the earlier occurrence first so that each + // member appears at most once and the slice stays bounded by the + // number of connections in the set. The tail check above already + // handled the case where c is the current tail. + if i := slices.Index(ds.sendHistory, c); i >= 0 { + ds.sendHistory = slices.Delete(ds.sendHistory, i, i+1) + } ds.sendHistory = append(ds.sendHistory, c) } diff --git a/derp/derp_test.go b/derp/derp_test.go index 7d8ea5f80191d..59a87d67a2878 100644 --- a/derp/derp_test.go +++ b/derp/derp_test.go @@ -1285,6 +1285,104 @@ func TestServerDupClients(t *testing.T) { }) } +// TestServerDupClientSendHistoryBounded verifies that a dup client set's +// sendHistory does not grow without bound when two connections sharing a +// key take turns sending frames. See the sendHistory field comment. +func TestServerDupClientSendHistoryBounded(t *testing.T) { + clientPub := key.NewNode().Public() + + newClient := func(t *testing.T, name string) *sclient { + return &sclient{key: clientPub, logf: logger.WithPrefix(t.Logf, name+": ")} + } + wantDupSet := func(t *testing.T, s *Server) *dupClientSet { + t.Helper() + dup, ok := s.clients[clientPub].(*dupClientSet) + if !ok { + t.Fatalf("wanted dup set; got %T", s.clients[clientPub]) + } + return dup + } + + t.Run("last_writer_active", func(t *testing.T) { + s := NewServer(key.NewNode(), t.Logf) + s.dupPolicy = lastWriterIsActive + c1, c2 := newClient(t, "c1"), newClient(t, "c2") + s.registerClient(c1) + s.registerClient(c2) + + dup := wantDupSet(t, s) + for range 5000 { + s.noteClientActivity(c1) + s.noteClientActivity(c2) + } + if got := len(dup.sendHistory); got > len(dup.set) { + t.Fatalf("sendHistory len = %d; want <= %d (set size)", got, len(dup.set)) + } + // The clients must remain active and undisabled, and the last + // one to send is the active one. + if c1.isDisabled.Load() || c2.isDisabled.Load() { + t.Fatal("client disabled under lastWriterIsActive; want none disabled") + } + if got := dup.ActiveClient(); got != c2 { + t.Fatalf("active client = %p; want c2 %p", got, c2) + } + + // When the active connection drops, the surviving one must + // still be picked up. + s.unregisterClient(c2) + if got := s.clients[clientPub].ActiveClient(); got != c1 { + t.Fatalf("after dropping c2, active = %p; want c1 %p", got, c1) + } + }) + + t.Run("history_order_survives_removal", func(t *testing.T) { + s := NewServer(key.NewNode(), t.Logf) + s.dupPolicy = lastWriterIsActive + c1, c2, c3 := newClient(t, "c1"), newClient(t, "c2"), newClient(t, "c3") + s.registerClient(c1) + s.registerClient(c2) + s.registerClient(c3) + + dup := wantDupSet(t, s) + for range 5000 { + s.noteClientActivity(c1) + s.noteClientActivity(c2) + s.noteClientActivity(c3) + } + s.noteClientActivity(c2) + if got := len(dup.sendHistory); got > len(dup.set) { + t.Fatalf("sendHistory len = %d; want <= %d (set size)", got, len(dup.set)) + } + + // c2 spoke last, then c3. Dropping c2 must make c3 active. + s.unregisterClient(c2) + if got := s.clients[clientPub].ActiveClient(); got != c3 { + t.Fatalf("after dropping c2, active = %p; want c3 %p", got, c3) + } + }) + + t.Run("disable_fighters", func(t *testing.T) { + s := NewServer(key.NewNode(), t.Logf) + s.dupPolicy = disableFighters + c1, c2 := newClient(t, "c1"), newClient(t, "c2") + s.registerClient(c1) + s.registerClient(c2) + + dup := wantDupSet(t, s) + for range 5000 { + s.noteClientActivity(c1) + s.noteClientActivity(c2) + } + if got := len(dup.sendHistory); got > len(dup.set) { + t.Fatalf("sendHistory len = %d; want <= %d (set size)", got, len(dup.set)) + } + // Fighting connections must be disabled. + if !c1.isDisabled.Load() || !c2.isDisabled.Load() { + t.Fatal("fighting clients not disabled; want both disabled") + } + }) +} + func TestLimiter(t *testing.T) { rl := rate.NewLimiter(rate.Every(time.Minute), 100) for i := 0; i < 200; i++ {