From c6ba1a6a4abcb3c457a1c15c84ff11817059478a Mon Sep 17 00:00:00 2001 From: Dylan Huff Date: Sat, 24 Oct 2026 23:45:13 +0000 Subject: [PATCH] fix(derp): bound dup client sendHistory growth Two connections that share a node key and take turns sending frames made noteClientActivity append to dupClientSet.sendHistory on every frame. The slice was only trimmed when a member disconnected, so it grew without limit under the server's global mutex. DERP clients do not need to authenticate, so any client could trigger this against an embedded DERP server. Move an existing entry to the end instead of appending a duplicate, so sendHistory holds each member at most once. Ported from upstream tailscale/tailscale@ec5d51098ab7 (#21477). --- derp/derp_server.go | 22 +++++++--- derp/derp_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 5 deletions(-) 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++ {