fix(net/portmapper): release mappings and UPnP connections on Close

net/portmapper/portmapper.go

@@ -202,6 +202,9 @@ func (c *Client) Close() error {
 	}
 	c.closed = true
 	c.invalidateMappingsLocked(true)
+	if c.uPnPHTTPClient != nil {
+		c.uPnPHTTPClient.CloseIdleConnections()
+	}
 	// TODO: close some future ever-listening UDP socket(s),
 	// waiting for multicast announcements from router.
 	return nil
@@ -298,6 +301,16 @@ func (c *Client) invalidateMappingsLocked(releaseOld bool) {
 	c.uPnPMeta = uPnPDiscoResponse{}
 }
 
+// setMappingLocked stores m or releases it if closed. c.mu must be held.
+func (c *Client) setMappingLocked(m mapping) bool {
+	if c.closed {
+		m.Release(context.Background())
+		return false
+	}
+	c.mapping = m
+	return true
+}
+
 func (c *Client) sawPMPRecently() bool {
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -391,7 +404,7 @@ func (c *Client) GetCachedMappingOrStartCreatingOne(ctx context.Context) (extern
 //
 // c.mu must be held.
 func (c *Client) maybeStartMappingLocked(ctx context.Context) {
-	if !c.runningCreate {
+	if !c.closed && !c.runningCreate {
 		c.runningCreate = true
 		go c.createMapping(ctx)
 	}
@@ -587,7 +600,9 @@ func (c *Client) createOrGetMapping(ctx context.Context) (external netip.AddrPor
 				pcpMapping.gw = netip.AddrPortFrom(gw, c.pxpPort())
 				c.mu.Lock()
 				defer c.mu.Unlock()
-				c.mapping = pcpMapping
+				if !c.setMappingLocked(pcpMapping) {
+					return netip.AddrPort{}, NoMappingError{net.ErrClosed}
+				}
 				return pcpMapping.external, nil
 			default:
 				c.logf("unknown PMP/PCP version number: %d %v", version, res[:n])
@@ -598,7 +613,9 @@ func (c *Client) createOrGetMapping(ctx context.Context) (external netip.AddrPor
 		if m.externalValid() {
 			c.mu.Lock()
 			defer c.mu.Unlock()
-			c.mapping = m
+			if !c.setMappingLocked(m) {
+				return netip.AddrPort{}, NoMappingError{net.ErrClosed}
+			}
 			return m.external, nil
 		}
 	}

net/portmapper/portmapper_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"reflect"
 	"strconv"
+	"sync/atomic"
 	"testing"
 	"time"
 )
@@ -122,3 +123,36 @@ func TestPCPIntegration(t *testing.T) {
 		t.Errorf("got nil mapping after successful createOrGetMapping")
 	}
 }
+
+type fakeMapping struct {
+	mapping
+	released atomic.Bool
+}
+
+func (m *fakeMapping) Release(context.Context) { m.released.Store(true) }
+
+func TestSetMappingLockedReleasesAfterClose(t *testing.T) {
+	t.Parallel()
+	c := NewClient(t.Logf, nil, nil, nil)
+	c.Close()
+	m := new(fakeMapping)
+	c.mu.Lock()
+	installed := c.setMappingLocked(m)
+	c.mu.Unlock()
+	if installed || c.mapping != nil || !m.released.Load() {
+		t.Errorf("setMappingLocked after Close: installed=%v, mapping=%v, released=%v; want false, nil, true",
+			installed, c.mapping, m.released.Load())
+	}
+}
+
+func TestNoMappingAttemptAfterClose(t *testing.T) {
+	t.Parallel()
+	c := NewClient(t.Logf, nil, nil, nil)
+	c.Close()
+	c.GetCachedMappingOrStartCreatingOne(context.Background())
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	if c.runningCreate {
+		t.Error("mapping attempt started after Close")
+	}
+}

net/portmapper/upnp.go

@@ -57,7 +57,14 @@ const upnpProtocolUDP = "UDP"
 func (u *upnpMapping) GoodUntil() time.Time     { return u.goodUntil }
 func (u *upnpMapping) RenewAfter() time.Time    { return u.renewAfter }
 func (u *upnpMapping) External() netip.AddrPort { return u.external }
+
+// upnpReleaseTimeout bounds DeletePortMapping, which callers run while
+// holding Client.mu.
+const upnpReleaseTimeout = time.Second
+
 func (u *upnpMapping) Release(ctx context.Context) {
+	ctx, cancel := context.WithTimeout(ctx, upnpReleaseTimeout)
+	defer cancel()
 	u.client.DeletePortMapping(ctx, "", u.external.Port(), upnpProtocolUDP)
 }
 
@@ -271,6 +278,16 @@ func (c *Client) getUPnPPortMapping(
 	meta := c.uPnPMeta
 	httpClient := c.upnpHTTPClientLocked()
 	c.mu.Unlock()
+
+	// Requests still in flight when Close runs return their connections to
+	// the pool afterwards, so close them once this attempt is done.
+	defer func() {
+		c.mu.Lock()
+		defer c.mu.Unlock()
+		if c.closed {
+			httpClient.CloseIdleConnections()
+		}
+	}()
 	if ok && oldMapping != nil {
 		client = oldMapping.client
 	} else {
@@ -287,6 +304,22 @@ func (c *Client) getUPnPPortMapping(
 		return netip.AddrPort{}, false
 	}
 
+	// Obtain and validate the external IP before creating the mapping. In
+	// particular, MikroTik only supports permanent leases, so forgetting a
+	// successfully-created mapping after a later request fails leaks the rule
+	// forever and each retry creates another one.
+	extIP, err := client.GetExternalIPAddress(ctx)
+	if c.debug.VerboseLogs {
+		c.logf("client.GetExternalIPAddress: %v, %v", extIP, err)
+	}
+	if err != nil {
+		return netip.AddrPort{}, false
+	}
+	externalIP, err := netip.ParseAddr(extIP)
+	if err != nil {
+		return netip.AddrPort{}, false
+	}
+
 	var newPort uint16
 	newPort, err = addAnyPortMapping(
 		ctx,
@@ -302,19 +335,6 @@ func (c *Client) getUPnPPortMapping(
 	if err != nil {
 		return netip.AddrPort{}, false
 	}
-	// TODO cache this ip somewhere?
-	extIP, err := client.GetExternalIPAddress(ctx)
-	if c.debug.VerboseLogs {
-		c.logf("client.GetExternalIPAddress: %v, %v", extIP, err)
-	}
-	if err != nil {
-		// TODO this doesn't seem right
-		return netip.AddrPort{}, false
-	}
-	externalIP, err := netip.ParseAddr(extIP)
-	if err != nil {
-		return netip.AddrPort{}, false
-	}
 
 	upnp.external = netip.AddrPortFrom(externalIP, newPort)
 	d := time.Duration(pmpMapLifetimeSec) * time.Second
@@ -323,7 +343,9 @@ func (c *Client) getUPnPPortMapping(
 	upnp.client = client
 	c.mu.Lock()
 	defer c.mu.Unlock()
-	c.mapping = upnp
+	if !c.setMappingLocked(upnp) {
+		return netip.AddrPort{}, false
+	}
 	c.localPort = newPort
 	return upnp.external, true
 }

net/portmapper/upnp_test.go

@@ -4,6 +4,7 @@
 package portmapper
 
 import (
+	"cmp"
 	"context"
 	"fmt"
 	"io"
@@ -13,7 +14,12 @@ import (
 	"net/netip"
 	"reflect"
 	"regexp"
+	"slices"
+	"strings"
+	"sync"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"tailscale.com/tstest"
 )
@@ -129,3 +135,143 @@ func TestGetUPnPClient(t *testing.T) {
 		})
 	}
 }
+
+func TestUPnPMappingLifecycle(t *testing.T) {
+	t.Parallel()
+	soap := func(action, args string) string {
+		return `<?xml version="1.0"?><s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/"><s:Body><u:` + action +
+			`Response xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">` + args + `</u:` + action + `Response></s:Body></s:Envelope>`
+	}
+	all := []string{"/rootDesc.xml", "GetExternalIPAddress", "AddPortMapping", "DeletePortMapping"}
+
+	for _, tt := range []struct {
+		name           string
+		extIP          string // GetExternalIPAddress result; "" returns HTTP 500
+		closeDuringAdd bool
+		hangDelete     bool
+		wantOK         bool
+		want           []string
+	}{
+		{name: "Close", extIP: "123.123.123.123", wantOK: true, want: all},
+		{name: "CloseDuringAdd", extIP: "123.123.123.123", closeDuringAdd: true, want: all},
+		{name: "ExternalIPError", want: all[:2]},
+		{name: "InvalidExternalIP", extIP: "invalid", want: all[:2]},
+		{name: "DeleteHangs", extIP: "123.123.123.123", hangDelete: true, wantOK: true, want: all},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+			defer cancel()
+
+			var mu sync.Mutex
+			var got []string
+			addEntered, addRelease := make(chan struct{}), make(chan struct{})
+			ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				_, action, _ := strings.Cut(strings.Trim(r.Header.Get("SOAPACTION"), `"`), "#")
+				mu.Lock()
+				got = append(got, cmp.Or(action, r.URL.Path))
+				mu.Unlock()
+				switch action {
+				case "":
+					io.WriteString(w, pfSenseRootDescXML)
+					return
+				case "GetExternalIPAddress":
+					if tt.extIP == "" {
+						w.WriteHeader(http.StatusInternalServerError)
+						return
+					}
+					io.WriteString(w, soap(action, "<NewExternalIPAddress>"+tt.extIP+"</NewExternalIPAddress>"))
+					return
+				case "AddPortMapping":
+					if tt.closeDuringAdd {
+						close(addEntered)
+						<-addRelease
+					}
+				case "DeletePortMapping":
+					if tt.hangDelete {
+						// The server only notices the client hanging up once the body is consumed.
+						io.Copy(io.Discard, r.Body)
+						<-r.Context().Done()
+						return
+					}
+				}
+				io.WriteString(w, soap(action, ""))
+			}))
+			var open atomic.Int32
+			ts.Config.ConnState = func(_ net.Conn, s http.ConnState) {
+				switch s {
+				case http.StateNew:
+					open.Add(1)
+				case http.StateClosed, http.StateHijacked:
+					open.Add(-1)
+				}
+			}
+			ts.Start()
+			t.Cleanup(ts.Close)
+
+			lo := netip.MustParseAddr("127.0.0.1")
+			c := NewClient(tstest.WhileTestRunningLogger(t), nil, &DebugKnobs{DisablePMP: true, DisablePCP: true}, nil)
+			c.SetGatewayLookupFunc(func() (gw, myIP netip.Addr, ok bool) { return lo, lo, true })
+			c.SetLocalPort(41641)
+			// Without an idle timeout, connections only close if Close
+			// closes them.
+			c.uPnPHTTPClient = &http.Client{Transport: &http.Transport{}}
+			c.gatewayAndSelfIP() // record the gateway so later lookups keep uPnPMeta
+			c.mu.Lock()
+			c.uPnPMeta = uPnPDiscoResponse{Location: ts.URL + "/rootDesc.xml"}
+			c.mu.Unlock()
+			t.Cleanup(func() { c.Close() })
+
+			done := make(chan error, 1)
+			go func() {
+				_, err := c.createOrGetMapping(ctx)
+				done <- err
+			}()
+			if tt.closeDuringAdd {
+				select {
+				case <-addEntered:
+				case <-ctx.Done():
+					t.Fatal("AddPortMapping not received")
+				}
+				c.Close()
+				close(addRelease)
+			}
+			var err error
+			select {
+			case err = <-done:
+			case <-ctx.Done():
+				t.Fatal("createOrGetMapping did not return")
+			}
+			if (err == nil) != tt.wantOK {
+				t.Fatalf("createOrGetMapping error = %v; want success %v", err, tt.wantOK)
+			}
+
+			closed := make(chan struct{})
+			go func() {
+				c.Close()
+				close(closed)
+			}()
+			select {
+			case <-closed:
+			case <-ctx.Done():
+				ts.CloseClientConnections() // unblock Close so cleanup can finish
+				t.Fatal("Close did not return")
+			}
+			if c.mapping != nil {
+				t.Errorf("mapping %v retained after Close", c.mapping.External())
+			}
+			mu.Lock()
+			defer mu.Unlock()
+			if !slices.Equal(got, tt.want) {
+				t.Errorf("requests = %v; want %v", got, tt.want)
+			}
+			for open.Load() > 0 {
+				select {
+				case <-time.After(10 * time.Millisecond):
+				case <-ctx.Done():
+					t.Fatalf("%d UPnP connections open after Close", open.Load())
+				}
+			}
+		})
+	}
+}