From 1e7c31d15f60f2e6a0aaf0dc4c49da58fd55ee15 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 15 Oct 2026 11:48:37 +0000 Subject: [PATCH 1/3] fix(net/portmapper): avoid leaking UPnP mappings on failed IP lookups Query and validate the external IP before AddPortMapping, so a failed lookup cannot leave an untracked mapping on the router. This ports the reordering from upstream commit 91d10d38ae ("net/portmapper: avoid leaking permanent UPnP mappings"). The fork has neither selectBestService nor the permanent-lease retry, so the rest of that commit and its tests do not apply; the next commit covers this behavior in TestUPnPMappingLifecycle. (cherry picked from commit 91d10d38ae58b8aa88db9e951342a00b76cbb625) --- net/portmapper/upnp.go | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 6a525c54ff297..9ce62c8fa5080 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -287,6 +287,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 +318,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 From a67dfec1df0b40898829538dd473916fba42df22 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 15 Oct 2026 11:49:02 +0000 Subject: [PATCH 2/3] fix(net/portmapper): clean up mappings and UPnP connections on Close Stop starting mapping attempts after Close, and release mappings that complete after Close instead of storing them. Close idle UPnP HTTP connections on Close, and again when a mapping attempt that was in flight during Close finishes, so no pooled connections outlive the client. --- net/portmapper/portmapper.go | 23 +++++- net/portmapper/portmapper_test.go | 34 ++++++++ net/portmapper/upnp.go | 14 +++- net/portmapper/upnp_test.go | 127 ++++++++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 4 deletions(-) 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 9ce62c8fa5080..29f3ac5628f30 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -271,6 +271,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 { @@ -326,7 +336,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..7f531aac3a945 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,124 @@ 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 + 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]}, + } { + 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) + case "GetExternalIPAddress": + if tt.extIP == "" { + w.WriteHeader(http.StatusInternalServerError) + return + } + io.WriteString(w, soap(action, ""+tt.extIP+"")) + case "AddPortMapping": + if tt.closeDuringAdd { + close(addEntered) + <-addRelease + } + fallthrough + default: + 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) + } + + c.Close() + 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()) + } + } + }) + } +} From af402204fabc111552940caf0d702992ec191fd2 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 15 Oct 2026 11:49:02 +0000 Subject: [PATCH 3/3] fix(net/portmapper): bound UPnP mapping release Release runs while holding Client.mu, so an unresponsive router could block Close, SetLocalPort, and gateway changes indefinitely. Limit DeletePortMapping to one second, matching the UPnP discovery fetch. --- net/portmapper/upnp.go | 7 +++++++ net/portmapper/upnp_test.go | 27 +++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 29f3ac5628f30..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) } diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go index 7f531aac3a945..f339056dc32c1 100644 --- a/net/portmapper/upnp_test.go +++ b/net/portmapper/upnp_test.go @@ -148,6 +148,7 @@ func TestUPnPMappingLifecycle(t *testing.T) { name string extIP string // GetExternalIPAddress result; "" returns HTTP 500 closeDuringAdd bool + hangDelete bool wantOK bool want []string }{ @@ -155,6 +156,7 @@ func TestUPnPMappingLifecycle(t *testing.T) { {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() @@ -172,21 +174,28 @@ func TestUPnPMappingLifecycle(t *testing.T) { 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 } - fallthrough - default: - io.WriteString(w, soap(action, "")) + 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) { @@ -237,7 +246,17 @@ func TestUPnPMappingLifecycle(t *testing.T) { t.Fatalf("createOrGetMapping error = %v; want success %v", err, tt.wantOK) } - c.Close() + 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()) }