diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go
index 3064eaf54b98e..239b91345fc80 100644
--- a/net/portmapper/portmapper.go
+++ b/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
}
}
diff --git a/net/portmapper/portmapper_test.go b/net/portmapper/portmapper_test.go
index 15f2849fc8d35..d21c81d20968f 100644
--- a/net/portmapper/portmapper_test.go
+++ b/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")
+ }
+}
diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go
index 6a525c54ff297..a46bc5729429f 100644
--- a/net/portmapper/upnp.go
+++ b/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
}
diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go
index c962a123c2b95..f339056dc32c1 100644
--- a/net/portmapper/upnp_test.go
+++ b/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 `` + args + ``
+ }
+ 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, ""+tt.extIP+""))
+ 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())
+ }
+ }
+ })
+ }
+}