From 6d8a414c06e3b26e174e554a259692635d1e3b9c Mon Sep 17 00:00:00 2001
From: Nick Khyl <nickk@tailscale.com>
Date: Tue, 29 Apr 2025 12:23:27 -0500
Subject: [PATCH] ipn/ipnlocal: fix taildrop deadlock

We update (*localNodeContext).AppendMatchingPeers's predicate to take a short-lived
PeerInfo struct instead of tailcfg.NodeView. This allows the predicate to use (*PeerInfo).HasCap(),
instead of calling back to localNodeContext causing the deadlock.

Fixes #15824

Signed-off-by: Nick Khyl <nickk@tailscale.com>
---
 ipn/ipnlocal/local.go    | 46 ++++++++++++++++++++++++++++++++++++++--
 ipn/ipnlocal/taildrop.go |  8 +++----
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index 95fe22641..bdccfb5c8 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -1463,18 +1463,60 @@ func (b *LocalBackend) PeerCaps(src netip.Addr) tailcfg.PeerCapMap {
 	return b.currentNode().PeerCaps(src)
 }
 
-func (b *localNodeContext) AppendMatchingPeers(base []tailcfg.NodeView, pred func(tailcfg.NodeView) bool) []tailcfg.NodeView {
+// PeerContext provides contextual information about a peer.
+type PeerContext interface {
+	Node() tailcfg.NodeView
+	Caps() tailcfg.PeerCapMap
+}
+
+// peerContext is a short-lived struct containing information about a peer.
+// It is not safe for concurrent use and [localNodeContext]'s mutex must be held
+// when using it. Therefore, it must not be retained or used outside the scope
+// of the predicate or callback it was passed to.
+type peerContext struct {
+	self *localNodeContext
+	peer tailcfg.NodeView
+}
+
+func (p peerContext) Node() tailcfg.NodeView {
+	if p.self == nil {
+		panic("peerInfo used outside of its scope")
+	}
+	return p.peer
+}
+
+// HasCap reports whether the peer has the specified capability.
+// It panics if used outside of the PeerInfo's scope.
+func (p peerContext) Caps() tailcfg.PeerCapMap {
+	if p.self == nil {
+		panic("peerInfo used outside of its scope")
+	}
+	if p.peer.Addresses().Len() == 0 {
+		return nil
+	}
+	return p.self.peerCapsLocked(p.peer.Addresses().At(0).Addr())
+}
+
+// AppendMatchingPeers appends to base all peers in the netmap that match the predicate.
+// The predicate is called with the node context's internal mutex locked,
+// so it must not call back into the backend or the node context.
+// The PeerInfo struct is only valid during the predicate call,
+// and must not be retained or used outside of it.
+func (b *localNodeContext) AppendMatchingPeers(base []tailcfg.NodeView, pred func(PeerContext) bool) []tailcfg.NodeView {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 	ret := base
 	if b.netMap == nil {
 		return ret
 	}
+	pi := &peerContext{self: b}
 	for _, peer := range b.netMap.Peers {
-		if pred(peer) {
+		pi.peer = peer
+		if pred(pi) {
 			ret = append(ret, peer)
 		}
 	}
+	pi.self = nil
 	return ret
 }
 
diff --git a/ipn/ipnlocal/taildrop.go b/ipn/ipnlocal/taildrop.go
index 17ca40926..8e4c6d394 100644
--- a/ipn/ipnlocal/taildrop.go
+++ b/ipn/ipnlocal/taildrop.go
@@ -190,14 +190,14 @@ func (b *LocalBackend) FileTargets() ([]*apitype.FileTarget, error) {
 	if !b.capFileSharing {
 		return nil, errors.New("file sharing not enabled by Tailscale admin")
 	}
-	peers := cn.AppendMatchingPeers(nil, func(p tailcfg.NodeView) bool {
-		if !p.Valid() || p.Hostinfo().OS() == "tvOS" {
+	peers := cn.AppendMatchingPeers(nil, func(p PeerContext) bool {
+		if !p.Node().Valid() || p.Node().Hostinfo().OS() == "tvOS" {
 			return false
 		}
-		if self != p.User() {
+		if self != p.Node().User() {
 			return false
 		}
-		if p.Addresses().Len() != 0 && cn.PeerHasCap(p.Addresses().At(0).Addr(), tailcfg.PeerCapabilityFileSharingTarget) {
+		if p.Caps().HasCapability(tailcfg.PeerCapabilityFileSharingTarget) {
 			// Explicitly noted in the netmap ACL caps as a target.
 			return true
 		}