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()) + } + } + }) + } +}