From 6393b0046f774afbf55679890bb62bcb7d8d9577 Mon Sep 17 00:00:00 2001
From: awdemos <awdemos@users.noreply.github.com>
Date: Thu, 27 Aug 2026 06:31:03 -0500
Subject: [PATCH] tailcat: make Server.Close and Client.Close idempotent

Close on either side could be called more than once, especially from
multiple cleanup paths. The underlying Engine.Close and NetMon.Close
are not documented as safe to call repeatedly, so a second Close could
panic or return an error. Guard both with a sync.Once so the second
and subsequent calls are harmless no-ops.

Add TestDoubleClose to verify that repeated Close calls on a started
Server and Client do not fail.

Updates tailscale/tailcat (adversarial audit).
---
 tailcat.go      | 16 ++++++++++++++--
 tailcat_test.go | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tailcat.go b/tailcat.go
index a0eba13c4..f7d6085ef 100644
--- a/tailcat.go
+++ b/tailcat.go
@@ -297,6 +297,8 @@ type Server struct {
 
 	lb *locoBackend // non-nil once Start has been called
 
+	closeOnce sync.Once
+
 	// AllowProxy, if non-nil, reports whether
 	// a TCP or UDP proxy is allowed for that target.
 	AllowProxy func(netip.AddrPort) bool
@@ -519,7 +521,11 @@ func (s *Server) Close() error {
 	if s.lb == nil {
 		return nil // never started
 	}
-	return s.lb.Close()
+	var err error
+	s.closeOnce.Do(func() {
+		err = s.lb.Close()
+	})
+	return err
 }
 
 // DrainTCP waits until every TCP connection in the server's netstack
@@ -1411,6 +1417,8 @@ type Client struct {
 	key     key.NodePrivate // the effective node identity; Key or generated
 	started bool
 
+	closeOnce sync.Once
+
 	upDone atomic.Bool // whether the server has meowed us at least once
 }
 
@@ -1552,7 +1560,11 @@ func (c *Client) Close() error {
 	if c.lb == nil {
 		return nil // never used
 	}
-	return c.lb.Close()
+	var err error
+	c.closeOnce.Do(func() {
+		err = c.lb.Close()
+	})
+	return err
 }
 
 // PingResult is the result of a successful [Client.Ping] call.
diff --git a/tailcat_test.go b/tailcat_test.go
index f6089a855..786fd6d24 100644
--- a/tailcat_test.go
+++ b/tailcat_test.go
@@ -381,3 +381,37 @@ func TestFetchDERPMapMemoryCache(t *testing.T) {
 		t.Errorf("fetches = %d; want 1", n)
 	}
 }
+
+// TestDoubleClose verifies that calling Close more than once on a
+// [Server] or [Client] is harmless and does not panic or error.
+func TestDoubleClose(t *testing.T) {
+	t.Parallel()
+
+	dm := integration.RunDERPAndSTUN(t, mkLogger(t, "derpstun"), "127.0.0.1")
+	reg := dm.Regions[1]
+	if reg == nil {
+		t.Fatal("no region 1 in derpmap")
+	}
+
+	s := &Server{Logf: mkLogger(t, "server"), Region: reg}
+	if err := s.Start(); err != nil {
+		t.Fatalf("server Start: %v", err)
+	}
+
+	c := &Client{Server: s.ConnBlob(), Logf: mkLogger(t, "client")}
+	PingForTest(t, s, c)
+
+	if err := c.Close(); err != nil {
+		t.Fatalf("first client Close: %v", err)
+	}
+	if err := c.Close(); err != nil {
+		t.Fatalf("second client Close: %v", err)
+	}
+
+	if err := s.Close(); err != nil {
+		t.Fatalf("first server Close: %v", err)
+	}
+	if err := s.Close(); err != nil {
+		t.Fatalf("second server Close: %v", err)
+	}
+}