TheArchive/tailscale
1
0
Fork 0
mirror of https://github.com/tailscale/tailscale.git synced 2024-11-25 11:05:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

net/netmon: fix goroutine leak in winMon if the monitor is never started

Browse Source 
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>
This commit is contained in:
Nick Khyl 2023-12-21 11:56:54 -06:00 committed by Nick Khyl
parent 2e956713de
commit c9836b454d
2 changed files with 30 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
net/netmon/netmon.go
View File

@ -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() {

24
net/netmon/netmon_windows.go
View File

@ -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) {