diff --git a/pkg/util/http/server.go b/pkg/util/http/server.go index 0bca8993..dac51779 100644 --- a/pkg/util/http/server.go +++ b/pkg/util/http/server.go @@ -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 } diff --git a/pkg/util/http/server_test.go b/pkg/util/http/server_test.go new file mode 100644 index 00000000..20a3b7e4 --- /dev/null +++ b/pkg/util/http/server_test.go @@ -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()) +} diff --git a/server/service.go b/server/service.go index 4556358b..a21c4f69 100644 --- a/server/service.go +++ b/server/service.go @@ -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 } diff --git a/server/service_lifecycle_test.go b/server/service_lifecycle_test.go index 61481b32..bf74ba69 100644 --- a/server/service_lifecycle_test.go +++ b/server/service_lifecycle_test.go @@ -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) {