From c9836b454d2388205bc71ff4c3c4c44ac4305604 Mon Sep 17 00:00:00 2001
From: Nick Khyl <nickk@tailscale.com>
Date: Thu, 21 Dec 2023 11:56:54 -0600
Subject: [PATCH] net/netmon: fix goroutine leak in winMon if the monitor is
 never started

When the portable Monitor creates a winMon via newOSMon, we register
address and route change callbacks with Windows. Once a callback is hit,
it starts a goroutine that attempts to send the event into messagec and returns.
The newly started goroutine then blocks until it can send to the channel.
However, if the monitor is never started and winMon.Receive is never called,
the goroutines remain indefinitely blocked, leading to goroutine leaks and
significant memory consumption in the tailscaled service process on Windows.
Unlike the tailscaled subprocess, the service process creates but never starts
a Monitor.

This PR adds a check within the callbacks to confirm the monitor's active status,
and exits immediately if the monitor hasn't started.

Updates #9864

Signed-off-by: Nick Khyl <nickk@tailscale.com>
---
 net/netmon/netmon.go         |  7 +++++++
 net/netmon/netmon_windows.go | 24 +++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/net/netmon/netmon.go b/net/netmon/netmon.go
index b0872034a..94a5de72f 100644
--- a/net/netmon/netmon.go
+++ b/net/netmon/netmon.go
@@ -220,6 +220,13 @@ func (m *Monitor) RegisterRuleDeleteCallback(callback RuleDeleteCallback) (unreg
 	}
 }
 
+// isActive reports whether this monitor has been started and not yet closed.
+func (m *Monitor) isActive() bool {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	return m.started && !m.closed
+}
+
 // Start starts the monitor.
 // A monitor can only be started & closed once.
 func (m *Monitor) Start() {
diff --git a/net/netmon/netmon_windows.go b/net/netmon/netmon_windows.go
index 836922206..a45452390 100644
--- a/net/netmon/netmon_windows.go
+++ b/net/netmon/netmon_windows.go
@@ -29,6 +29,7 @@ type winMon struct {
 	logf                  logger.Logf
 	ctx                   context.Context
 	cancel                context.CancelFunc
+	isActive              func() bool
 	messagec              chan eventMessage
 	addressChangeCallback *winipcfg.UnicastAddressChangeCallback
 	routeChangeCallback   *winipcfg.RouteChangeCallback
@@ -44,9 +45,10 @@ type winMon struct {
 	noDeadlockTicker *time.Ticker
 }
 
-func newOSMon(logf logger.Logf, _ *Monitor) (osMon, error) {
+func newOSMon(logf logger.Logf, pm *Monitor) (osMon, error) {
 	m := &winMon{
 		logf:             logf,
+		isActive:         pm.isActive,
 		messagec:         make(chan eventMessage, 1),
 		noDeadlockTicker: time.NewTicker(5000 * time.Hour), // arbitrary
 	}
@@ -130,6 +132,16 @@ func (m *winMon) Receive() (message, error) {
 
 // unicastAddressChanged is the callback we register with Windows to call when unicast address changes.
 func (m *winMon) unicastAddressChanged(_ winipcfg.MibNotificationType, row *winipcfg.MibUnicastIPAddressRow) {
+	if !m.isActive() {
+		// Avoid starting a goroutine that sends events to messagec,
+		// or sending messages to messagec directly, if the monitor
+		// hasn't started and Receive is not yet reading from messagec.
+		//
+		// Doing so can lead to goroutine leaks or deadlocks, especially
+		// if the monitor is never started.
+		return
+	}
+
 	what := "addr"
 	if ip := row.Address.Addr(); ip.IsValid() && tsaddr.IsTailscaleIP(ip.Unmap()) {
 		what = "tsaddr"
@@ -141,6 +153,16 @@ func (m *winMon) unicastAddressChanged(_ winipcfg.MibNotificationType, row *wini
 
 // routeChanged is the callback we register with Windows to call when route changes.
 func (m *winMon) routeChanged(_ winipcfg.MibNotificationType, row *winipcfg.MibIPforwardRow2) {
+	if !m.isActive() {
+		// Avoid starting a goroutine that sends events to messagec,
+		// or sending messages to messagec directly, if the monitor
+		// hasn't started and Receive is not yet reading from messagec.
+		//
+		// Doing so can lead to goroutine leaks or deadlocks, especially
+		// if the monitor is never started.
+		return
+	}
+
 	what := "route"
 	ip := row.DestinationPrefix.Prefix().Addr().Unmap()
 	if ip.IsValid() && tsaddr.IsTailscaleIP(ip) {