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 <sfllaw@tailscale.com>

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")
 	}

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)