fix: release listeners when frps fails to start (#5574)

A constructor that binds a port before its fallible preparation leaks the listener when a later step returns an error, and no one can close it because the returned service is nil. pkg/util/http: load the TLS keypair and build the auth middleware before net.Listen, so binding is the last step that can run and a failure leaves no bound port behind. server: create the web server after auth, the tcpmux, main, kcp, quic, ssh gateway and vhost listeners, and the nat hole controller, following what frpc already does. If the web server itself fails, close the resources created so far before returning. The port managers start their cleanup workers on creation, so they are created after every fallible step too, and the metrics collectors keep being enabled only when the web server is configured. Add regression tests for all three failure paths.
2026-10-19 18:13:28 +08:00 · 2026-10-19 12:20:25 +08:00
parent 26742060b1
commit 399bf2ce4c
4 changed files with 255 additions and 29 deletions
@@ -53,11 +53,6 @@ func NewServer(cfg v1.WebServerConfig) (*Server, error) {
 		addr = ":http"
 	}

-	ln, err := net.Listen("tcp", addr)
-	if err != nil {
-		return nil, err
-	}
-
 	router := mux.NewRouter()
 	hs := &http.Server{
 		Addr:         addr,
@@ -67,7 +62,6 @@ func NewServer(cfg v1.WebServerConfig) (*Server, error) {
 	}
 	s := &Server{
 		addr:   addr,
-		ln:     ln,
 		hs:     hs,
 		router: router,
 	}
@@ -84,6 +78,13 @@ func NewServer(cfg v1.WebServerConfig) (*Server, error) {
 		}
 	}
 	s.authMiddleware = netpkg.NewHTTPAuthMiddleware(cfg.User, cfg.Password).SetAuthFailDelay(200 * time.Millisecond).Middleware
+
+	// Bind only after all fallible preparation so an error cannot leak the listener.
+	ln, err := net.Listen("tcp", addr)
+	if err != nil {
+		return nil, err
+	}
+	s.ln = ln
 	return s, nil
 }

@@ -0,0 +1,55 @@
+// Copyright 2026 The frp Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package http
+
+import (
+	"net"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	v1 "github.com/fatedier/frp/pkg/config/v1"
+)
+
+func TestNewServerDoesNotLeakListenerOnTLSFailure(t *testing.T) {
+	dir := t.TempDir()
+	certFile := filepath.Join(dir, "cert.pem")
+	keyFile := filepath.Join(dir, "key.pem")
+	require.NoError(t, os.WriteFile(certFile, []byte("invalid certificate"), 0o600))
+	require.NoError(t, os.WriteFile(keyFile, []byte("invalid key"), 0o600))
+
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	address := ln.Addr().String()
+	port := ln.Addr().(*net.TCPAddr).Port
+	require.NoError(t, ln.Close())
+
+	svr, err := NewServer(v1.WebServerConfig{
+		Addr: "127.0.0.1",
+		Port: port,
+		TLS:  &v1.TLSConfig{CertFile: certFile, KeyFile: keyFile},
+	})
+	if svr != nil {
+		defer svr.Close()
+	}
+	require.ErrorContains(t, err, "failed to find any PEM data in certificate input")
+	require.Nil(t, svr)
+
+	ln, err = net.Listen("tcp", address)
+	require.NoError(t, err, "web server port was not released")
+	require.NoError(t, ln.Close())
+}
@@ -152,20 +152,6 @@ func NewService(cfg *v1.ServerConfig) (*Service, error) {
 		return nil, err
 	}

-	var webServer *httppkg.Server
-	if cfg.WebServer.Port > 0 {
-		ws, err := httppkg.NewServer(cfg.WebServer)
-		if err != nil {
-			return nil, err
-		}
-		webServer = ws
-
-		modelmetrics.EnableMem()
-		if cfg.EnablePrometheus {
-			modelmetrics.EnablePrometheus()
-		}
-	}
-
 	authRuntime, err := auth.BuildServerAuth(&cfg.Auth)
 	if err != nil {
 		return nil, err
@@ -179,20 +165,14 @@ func NewService(cfg *v1.ServerConfig) (*Service, error) {
 		pluginManager:  plugin.NewManager(),
 		rc: &controller.ResourceController{
 			VisitorManager: visitor.NewManager(),
-			TCPPortManager: ports.NewManager("tcp", cfg.ProxyBindAddr, cfg.AllowPorts),
-			UDPPortManager: ports.NewManager("udp", cfg.ProxyBindAddr, cfg.AllowPorts),
 		},
 		sshTunnelListener: netpkg.NewInternalListener(),
 		httpVhostRouter:   vhost.NewRouters(),
 		auth:              authRuntime,
-		webServer:         webServer,
 		tlsConfig:         tlsConfig,
 		cfg:               cfg,
 		ctx:               context.Background(),
 	}
-	if webServer != nil {
-		webServer.RouteRegister(svr.registerRouteHandlers)
-	}

 	// Create tcpmux httpconnect multiplexer.
 	if cfg.TCPMuxHTTPConnectPort > 0 {
@@ -217,9 +197,6 @@ func NewService(cfg *v1.ServerConfig) (*Service, error) {
 	}
 	svr.rc.PluginManager = svr.pluginManager

-	// Init group controller
-	svr.rc.TCPGroupCtl = group.NewTCPGroupCtl(svr.rc.TCPPortManager)
-
 	// Init HTTP group controller
 	svr.rc.HTTPGroupCtl = group.NewHTTPGroupController(svr.httpVhostRouter)

@@ -369,6 +346,32 @@ func NewService(cfg *v1.ServerConfig) (*Service, error) {
 		return nil, fmt.Errorf("create nat hole controller error, %v", err)
 	}
 	svr.rc.NatHoleController = nc
+
+	// Create the web server after all fallible steps so its listener is not
+	// leaked when an earlier error causes NewService to return.
+	if cfg.WebServer.Port > 0 {
+		ws, err := httppkg.NewServer(cfg.WebServer)
+		if err != nil {
+			// All other listeners are already created and must be released on failure.
+			_ = svr.Close()
+			return nil, err
+		}
+		svr.webServer = ws
+
+		modelmetrics.EnableMem()
+		if cfg.EnablePrometheus {
+			modelmetrics.EnablePrometheus()
+		}
+	}
+
+	// Start port-manager cleanup workers only after all fallible steps have succeeded.
+	svr.rc.TCPPortManager = ports.NewManager("tcp", cfg.ProxyBindAddr, cfg.AllowPorts)
+	svr.rc.UDPPortManager = ports.NewManager("udp", cfg.ProxyBindAddr, cfg.AllowPorts)
+	svr.rc.TCPGroupCtl = group.NewTCPGroupCtl(svr.rc.TCPPortManager)
+
+	if svr.webServer != nil {
+		svr.webServer.RouteRegister(svr.registerRouteHandlers)
+	}
 	return svr, nil
 }

@@ -1,9 +1,14 @@
 package server

 import (
+	"bytes"
 	"context"
 	"fmt"
 	"net"
+	"os"
+	"path/filepath"
+	"runtime/pprof"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -86,6 +91,168 @@ func assertLifecyclePorts(t *testing.T, addresses []string, released bool) {
 	}
 }

+func TestNewServiceDoesNotLeakWebServerOnBindFailure(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	defer ln.Close()
+
+	cfg := &v1.ServerConfig{
+		BindAddr:      "127.0.0.1",
+		BindPort:      ln.Addr().(*net.TCPAddr).Port,
+		ProxyBindAddr: "127.0.0.1",
+		WebServer:     v1.WebServerConfig{Addr: "127.0.0.1", Port: freeLifecycleTCPPort(t)},
+	}
+	require.NoError(t, cfg.Complete())
+
+	svr, err := NewService(cfg)
+	if svr != nil {
+		defer svr.Close()
+	}
+	require.ErrorContains(t, err, "create server listener error")
+	require.ErrorContains(t, err, ln.Addr().String())
+	require.Nil(t, svr)
+	assertLifecyclePorts(t, []string{net.JoinHostPort(cfg.WebServer.Addr, fmt.Sprint(cfg.WebServer.Port))}, true)
+}
+
+func countPortManagerWorkers(t *testing.T) int {
+	t.Helper()
+	var profile bytes.Buffer
+	require.NoError(t, pprof.Lookup("goroutine").WriteTo(&profile, 2))
+	count := 0
+	for stack := range strings.SplitSeq(profile.String(), "\n\n") {
+		if strings.Contains(stack, "github.com/fatedier/frp/server/ports.(*Manager).cleanReservedPortsWorker") {
+			count++
+		}
+	}
+	return count
+}
+
+func TestNewServiceDoesNotLeakPortManagerWorkersOnWebServerFailure(t *testing.T) {
+	for _, mode := range []string{"port conflict", "invalid TLS certificate"} {
+		t.Run(mode, func(t *testing.T) {
+			ln, err := net.Listen("tcp", "127.0.0.1:0")
+			require.NoError(t, err)
+			defer ln.Close()
+			cfg := &v1.ServerConfig{
+				BindAddr:      "127.0.0.1",
+				ProxyBindAddr: "127.0.0.1",
+				WebServer:     v1.WebServerConfig{Addr: "127.0.0.1", Port: ln.Addr().(*net.TCPAddr).Port},
+			}
+			require.NoError(t, cfg.Complete())
+			cfg.BindPort = 0
+			if mode == "invalid TLS certificate" {
+				dir := t.TempDir()
+				certFile := filepath.Join(dir, "cert.pem")
+				keyFile := filepath.Join(dir, "key.pem")
+				require.NoError(t, os.WriteFile(certFile, []byte("invalid certificate"), 0o600))
+				require.NoError(t, os.WriteFile(keyFile, []byte("invalid key"), 0o600))
+				cfg.WebServer.TLS = &v1.TLSConfig{CertFile: certFile, KeyFile: keyFile}
+				require.NoError(t, ln.Close())
+			}
+
+			workersBefore := countPortManagerWorkers(t)
+			for range 5 {
+				svr, err := NewService(cfg)
+				if svr != nil {
+					defer svr.Close()
+				}
+				require.Nil(t, svr)
+				if mode == "port conflict" {
+					var listenErr *net.OpError
+					require.ErrorAs(t, err, &listenErr)
+					require.Equal(t, "listen", listenErr.Op)
+					require.Equal(t, ln.Addr().String(), listenErr.Addr.String())
+				} else {
+					require.ErrorContains(t, err, "failed to find any PEM data in certificate input")
+				}
+			}
+			workersAfter := countPortManagerWorkers(t)
+			t.Logf("port-manager workers after 5 failed constructions: before=%d after=%d", workersBefore, workersAfter)
+			require.Equal(t, workersBefore, workersAfter, "failed construction leaked port-manager workers")
+		})
+	}
+}
+
+func TestNewServiceReleasesListenersOnWebServerFailure(t *testing.T) {
+	for _, mode := range []string{"port conflict", "invalid TLS certificate"} {
+		t.Run(mode, func(t *testing.T) {
+			cfg := &v1.ServerConfig{
+				BindAddr:      "127.0.0.1",
+				ProxyBindAddr: "127.0.0.1",
+				WebServer:     v1.WebServerConfig{Addr: "127.0.0.1"},
+			}
+			require.NoError(t, cfg.Complete())
+			dir := t.TempDir()
+			cfg.SSHTunnelGateway.AutoGenPrivateKeyPath = filepath.Join(dir, "ssh_key")
+
+			// Hold reservations together so each listener gets a distinct port.
+			tcpPorts := []*int{
+				&cfg.BindPort, &cfg.TCPMuxHTTPConnectPort, &cfg.SSHTunnelGateway.BindPort,
+				&cfg.VhostHTTPPort, &cfg.VhostHTTPSPort, &cfg.WebServer.Port,
+			}
+			tcpReservations := make([]net.Listener, 0, len(tcpPorts))
+			tcpAddresses := make([]string, 0, len(tcpPorts))
+			for _, port := range tcpPorts {
+				ln, err := net.Listen("tcp", "127.0.0.1:0")
+				require.NoError(t, err)
+				t.Cleanup(func() { _ = ln.Close() })
+				*port = ln.Addr().(*net.TCPAddr).Port
+				tcpReservations = append(tcpReservations, ln)
+				tcpAddresses = append(tcpAddresses, ln.Addr().String())
+			}
+			udpPorts := []*int{&cfg.KCPBindPort, &cfg.QUICBindPort}
+			udpReservations := make([]net.PacketConn, 0, len(udpPorts))
+			udpAddresses := make([]string, 0, len(udpPorts))
+			for _, port := range udpPorts {
+				conn, err := net.ListenPacket("udp", "127.0.0.1:0")
+				require.NoError(t, err)
+				t.Cleanup(func() { _ = conn.Close() })
+				*port = conn.LocalAddr().(*net.UDPAddr).Port
+				udpReservations = append(udpReservations, conn)
+				udpAddresses = append(udpAddresses, conn.LocalAddr().String())
+			}
+
+			webReservation := tcpReservations[len(tcpReservations)-1]
+			if mode == "invalid TLS certificate" {
+				certFile := filepath.Join(dir, "cert.pem")
+				keyFile := filepath.Join(dir, "key.pem")
+				require.NoError(t, os.WriteFile(certFile, []byte("invalid certificate"), 0o600))
+				require.NoError(t, os.WriteFile(keyFile, []byte("invalid key"), 0o600))
+				cfg.WebServer.TLS = &v1.TLSConfig{CertFile: certFile, KeyFile: keyFile}
+				require.NoError(t, webReservation.Close())
+			}
+			for _, ln := range tcpReservations[:len(tcpReservations)-1] {
+				require.NoError(t, ln.Close())
+			}
+			for _, conn := range udpReservations {
+				require.NoError(t, conn.Close())
+			}
+
+			svr, err := NewService(cfg)
+			if svr != nil {
+				defer svr.Close()
+			}
+			require.Nil(t, svr)
+			if mode == "port conflict" {
+				var listenErr *net.OpError
+				require.ErrorAs(t, err, &listenErr)
+				require.Equal(t, "listen", listenErr.Op)
+				require.Equal(t, webReservation.Addr().String(), listenErr.Addr.String())
+				assertLifecyclePorts(t, []string{webReservation.Addr().String()}, false)
+				require.NoError(t, webReservation.Close())
+			} else {
+				require.ErrorContains(t, err, "failed to find any PEM data in certificate input")
+			}
+			assertLifecyclePorts(t, tcpAddresses, true)
+			for _, address := range udpAddresses {
+				conn, err := net.ListenPacket("udp", address)
+				require.NoError(t, err, "UDP listener %s was not released", address)
+				require.NoError(t, conn.Close())
+			}
+		})
+	}
+}
+
 func TestServiceRunAfterEarlyShutdown(t *testing.T) {
 	for _, mode := range []string{"close before Run", "close before Run with web", "parent already cancelled"} {
 		t.Run(mode, func(t *testing.T) {