From 4cccd15eeb13d5d7f8f831e5406a567b8f18378b Mon Sep 17 00:00:00 2001
From: Nick Khyl <nickk@tailscale.com>
Date: Thu, 29 May 2025 13:51:46 -0500
Subject: [PATCH] ipn/ipnlocal: fix data race when accessing b.appConnector

The field must only be accessed while holding LocalBackend's mutex,
but there are two places where it's accessed without the mutex:
 - (LocalBackend).MaybeClearAppConnector()
 - handleC2NAppConnectorDomainRoutesGet()

Fixes #16123

Signed-off-by: Nick Khyl <nickk@tailscale.com>
---
 ipn/ipnlocal/c2n.go   |  5 +++--
 ipn/ipnlocal/local.go | 17 ++++++++++++-----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/ipn/ipnlocal/c2n.go b/ipn/ipnlocal/c2n.go
index b33794751..876c13064 100644
--- a/ipn/ipnlocal/c2n.go
+++ b/ipn/ipnlocal/c2n.go
@@ -240,13 +240,14 @@ func handleC2NAppConnectorDomainRoutesGet(b *LocalBackend, w http.ResponseWriter
 	b.logf("c2n: GET /appconnector/routes received")
 
 	var res tailcfg.C2NAppConnectorDomainRoutesResponse
-	if b.appConnector == nil {
+	appConnector := b.AppConnector()
+	if appConnector == nil {
 		w.Header().Set("Content-Type", "application/json")
 		json.NewEncoder(w).Encode(res)
 		return
 	}
 
-	res.Domains = b.appConnector.DomainRoutes()
+	res.Domains = appConnector.DomainRoutes()
 
 	w.Header().Set("Content-Type", "application/json")
 	json.NewEncoder(w).Encode(res)
diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index d2f6c86f7..d69c07a9f 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -4150,8 +4150,8 @@ func (b *LocalBackend) SetUseExitNodeEnabled(v bool) (ipn.PrefsView, error) {
 // AdvertiseRoutes has been set in the MaskedPrefs.
 func (b *LocalBackend) MaybeClearAppConnector(mp *ipn.MaskedPrefs) error {
 	var err error
-	if b.appConnector != nil && mp.AdvertiseRoutesSet {
-		err = b.appConnector.ClearRoutes()
+	if ac := b.AppConnector(); ac != nil && mp.AdvertiseRoutesSet {
+		err = ac.ClearRoutes()
 		if err != nil {
 			b.logf("appc: clear routes error: %v", err)
 		}
@@ -4755,9 +4755,7 @@ func (b *LocalBackend) readvertiseAppConnectorRoutes() {
 	//
 	// Grab a copy of the field, since b.mu only guards access to the
 	// b.appConnector field itself.
-	b.mu.Lock()
-	appConnector := b.appConnector
-	b.mu.Unlock()
+	appConnector := b.AppConnector()
 
 	if appConnector == nil {
 		return
@@ -6432,6 +6430,15 @@ func (b *LocalBackend) OfferingAppConnector() bool {
 	return b.appConnector != nil
 }
 
+// AppConnector returns the current AppConnector, or nil if not configured.
+//
+// TODO(nickkhyl): move app connectors to [nodeBackend], or perhaps a feature package?
+func (b *LocalBackend) AppConnector() *appc.AppConnector {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return b.appConnector
+}
+
 // allowExitNodeDNSProxyToServeName reports whether the Exit Node DNS
 // proxy is allowed to serve responses for the provided DNS name.
 func (b *LocalBackend) allowExitNodeDNSProxyToServeName(name string) bool {