From e4e59a2af00f2927315024295bab765588824274 Mon Sep 17 00:00:00 2001 From: Simon Law Date: Wed, 13 May 2026 08:13:40 -0700 Subject: [PATCH] wgengine/netstack: stop inject goroutine from leaking in Impl.Start (#19721) This patch fixes a data race in wgengine/netstack that surfaced while running both TestTCPForwardLimits and TestTCPForwardLimits_PerClient. Because these two tests both setup the TS_DEBUG_NETSTACK envknob, a race happens because netstack.Impl.Close leaked its inject goroutine. The inject goroutine also reads the TS_DEBUG_NETSTACK envknob, so if it is still running when the next test starts, then it will break. This patch also cleans up the tests a bit, ensuring that neither of them run in T.Parallel. It also adds a T.Cleanup call to clear the envknob. Fixes #19720 Signed-off-by: Simon Law --- wgengine/netstack/netstack.go | 6 +++++- wgengine/netstack/netstack_test.go | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index 11900edbf..659e07924 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -216,6 +216,7 @@ type Impl struct { dialer *tsdial.Dialer ctx context.Context // alive until Close ctxCancel context.CancelFunc // called on Close + injectWG sync.WaitGroup // wait for the inject goroutine lb *ipnlocal.LocalBackend // or nil dns *dns.Manager @@ -450,6 +451,7 @@ func (ns *Impl) Close() error { ns.ctxCancel() ns.ipstack.Close() ns.ipstack.Wait() + ns.injectWG.Wait() return nil } @@ -644,7 +646,9 @@ func (ns *Impl) Start(b LocalBackend) error { udpFwd := udp.NewForwarder(ns.ipstack, ns.acceptUDPNoICMP) ns.ipstack.SetTransportProtocolHandler(tcp.ProtocolNumber, ns.wrapTCPProtocolHandler(tcpFwd.HandlePacket)) ns.ipstack.SetTransportProtocolHandler(udp.ProtocolNumber, ns.wrapUDPProtocolHandler(udpFwd.HandlePacket)) - go ns.inject() + ns.injectWG.Go(func() { + ns.inject() + }) if ns.ready.Swap(true) { panic("already started") } diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index 4f920c8e0..7f248cd44 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -737,7 +737,10 @@ func makeHangDialer(tb testing.TB) (netx.DialFunc, chan struct{}) { // TestTCPForwardLimits verifies that the limits on the TCP forwarder work in a // success case (i.e. when we don't hit the limit). func TestTCPForwardLimits(t *testing.T) { + tstest.AssertNotParallel(t) // calls envknob.Setenv envknob.Setenv("TS_DEBUG_NETSTACK", "true") + t.Cleanup(func() { envknob.Setenv("TS_DEBUG_NETSTACK", "") }) + impl := makeNetstack(t, func(impl *Impl) { impl.ProcessSubnets = true }) @@ -815,6 +818,7 @@ func TestTCPForwardLimits_PerClient(t *testing.T) { clientmetric.ResetForTest(t) tstest.AssertNotParallel(t) // calls envknob.Setenv envknob.Setenv("TS_DEBUG_NETSTACK", "true") + t.Cleanup(func() { envknob.Setenv("TS_DEBUG_NETSTACK", "") }) // Set our test override limits during this test. maxInFlightConnectionAttemptsForTest.Store(2)