diff --git a/client_handler_test.go b/client_handler_test.go index 56c74957..668aa3d3 100644 --- a/client_handler_test.go +++ b/client_handler_test.go @@ -3,6 +3,7 @@ package ftpserver import ( "fmt" "net" + "slices" "sync" "testing" "time" @@ -20,7 +21,7 @@ func TestConcurrency(t *testing.T) { waitGroup := sync.WaitGroup{} waitGroup.Add(nbClients) - for i := 0; i < nbClients; i++ { + for range nbClients { go func() { conf := goftp.Config{ User: authUser, @@ -47,7 +48,8 @@ func TestConcurrency(t *testing.T) { func TestDOS(t *testing.T) { server := NewTestServer(t, true) - conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second) + dialer := &net.Dialer{Timeout: 5 * time.Second} + conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr()) require.NoError(t, err) defer func() { @@ -149,7 +151,8 @@ func TestConnectionNotAllowed(t *testing.T) { } s := NewTestServerWithTestDriver(t, driver) - conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second) + dialer := &net.Dialer{Timeout: 5 * time.Second} + conn, err := dialer.DialContext(t.Context(), "tcp", s.Addr()) require.NoError(t, err) defer func() { @@ -315,13 +318,7 @@ second line } func isStringInSlice(s string, list []string) bool { - for _, c := range list { - if s == c { - return true - } - } - - return false + return slices.Contains(list, s) } func TestUnknownCommand(t *testing.T) { @@ -486,7 +483,7 @@ func TestExtraData(t *testing.T) { require.Len(t, info, 1) for k, v := range info { - ccInfo, ok := v.(map[string]interface{}) + ccInfo, ok := v.(map[string]any) require.True(t, ok) extra, ok := ccInfo["extra"].(uint32) require.True(t, ok) diff --git a/handle_auth.go b/handle_auth.go index 7c200abd..8b175bc8 100644 --- a/handle_auth.go +++ b/handle_auth.go @@ -11,7 +11,7 @@ func (c *clientHandler) handleUSER(user string) error { err := verifier.PreAuthUser(c, user) if err != nil { c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("User rejected: %v", err)) - c.disconnect() + _ = c.disconnect() return nil } @@ -19,7 +19,7 @@ func (c *clientHandler) handleUSER(user string) error { if c.isTLSRequired() && !c.HasTLSForControl() { c.writeMessage(StatusServiceNotAvailable, "TLS is required") - c.disconnect() + _ = c.disconnect() return nil } @@ -52,7 +52,7 @@ func (c *clientHandler) handleUserTLS(user string) bool { driver, err := verifier.VerifyConnection(c, user, tlsConn) if err != nil { c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("TLS verification failed: %v", err)) - c.disconnect() + _ = c.disconnect() return true } @@ -82,14 +82,14 @@ func (c *clientHandler) handlePASS(param string) error { switch { case err == nil && c.driver == nil: c.writeMessage(StatusNotLoggedIn, "Unexpected exception (driver is nil)") - c.disconnect() + _ = c.disconnect() case err != nil: if msg == "" { msg = fmt.Sprintf("Authentication error: %v", err) } c.writeMessage(StatusNotLoggedIn, msg) - c.disconnect() + _ = c.disconnect() default: // err == nil && c.driver != nil if msg == "" { msg = "Password ok, continue" diff --git a/handle_auth_test.go b/handle_auth_test.go index feae7899..bb6a4495 100644 --- a/handle_auth_test.go +++ b/handle_auth_test.go @@ -19,7 +19,8 @@ func panicOnError(err error) { func TestLoginSuccess(t *testing.T) { server := NewTestServer(t, false) // send a NOOP before the login, this doesn't seems possible using secsy/goftp so use the old way ... - conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second) + dialer := &net.Dialer{Timeout: 5 * time.Second} + conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr()) require.NoError(t, err) defer func() { diff --git a/handle_dirs_test.go b/handle_dirs_test.go index 698a7bc6..eb0d4d5f 100644 --- a/handle_dirs_test.go +++ b/handle_dirs_test.go @@ -460,7 +460,8 @@ func TestPerClientTLSTransfer(t *testing.T) { func TestDirListingBeforeLogin(t *testing.T) { s := NewTestServer(t, false) - conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second) + dialer := &net.Dialer{Timeout: 5 * time.Second} + conn, err := dialer.DialContext(t.Context(), "tcp", s.Addr()) require.NoError(t, err) defer func() { diff --git a/handle_files.go b/handle_files.go index eb09fc33..b2835c1f 100644 --- a/handle_files.go +++ b/handle_files.go @@ -689,7 +689,7 @@ func (c *clientHandler) computeHashForFile(filePath string, algo HASHAlgo, start case HASHAlgoSHA512: chosenHashAlgo = sha512.New() default: - return "", errUnknowHash + return "", errUnknownHash } file, err = c.getFileHandle(filePath, os.O_RDONLY, start) diff --git a/handle_misc.go b/handle_misc.go index 5444777d..33d6c34c 100644 --- a/handle_misc.go +++ b/handle_misc.go @@ -10,7 +10,7 @@ import ( "time" ) -var errUnknowHash = errors.New("unknown hash algorithm") +var errUnknownHash = errors.New("unknown hash algorithm") func (c *clientHandler) handleAUTH(_ string) error { if tlsConfig, err := c.server.driver.GetTLSConfig(); err == nil { @@ -316,7 +316,7 @@ func (c *clientHandler) handleQUIT(_ string) error { } c.writeMessage(StatusClosingControlConn, msg) - c.disconnect() + _ = c.disconnect() c.reader = nil return nil diff --git a/server.go b/server.go index a00f63c7..20d124dc 100644 --- a/server.go +++ b/server.go @@ -2,6 +2,7 @@ package ftpserver import ( + "context" "crypto/tls" "errors" "fmt" @@ -208,7 +209,8 @@ func (server *FtpServer) Listen() error { } func (server *FtpServer) createListener() (net.Listener, error) { - listener, err := net.Listen("tcp", server.settings.ListenAddr) + lc := &net.ListenConfig{} + listener, err := lc.Listen(context.Background(), "tcp", server.settings.ListenAddr) if err != nil { server.Logger.Error("cannot listen on main port", "err", err, "listenAddr", server.settings.ListenAddr) @@ -266,8 +268,6 @@ func (server *FtpServer) Serve() error { // It returns a boolean indicating if the error should stop the server and the error itself or none if it's a standard // scenario (e.g. a closed listener) func (server *FtpServer) handleAcceptError(err error, tempDelay *time.Duration) (bool, error) { - server.Logger.Error("Serve error", "err", err) - if errOp := (&net.OpError{}); errors.As(err, &errOp) { // This means we just closed the connection and it's OK if errOp.Err.Error() == "use of closed network connection" { @@ -277,6 +277,8 @@ func (server *FtpServer) handleAcceptError(err error, tempDelay *time.Duration) } } + server.Logger.Error("Serve error", "err", err) + // see https://github.com/golang/go/blob/4aa1efed4853ea067d665a952eee77c52faac774/src/net/http/server.go#L3046 // & https://github.com/fclairamb/ftpserverlib/pull/352#pullrequestreview-1077459896 // The temporaryError method should replace net.Error.Temporary() when the go team diff --git a/server_stop_test.go b/server_stop_test.go new file mode 100644 index 00000000..615b26d0 --- /dev/null +++ b/server_stop_test.go @@ -0,0 +1,90 @@ +package ftpserver + +import ( + "sync" + "testing" + "time" + + log "github.com/fclairamb/go-log" + "github.com/stretchr/testify/require" +) + +// TestServerStopDoesNotLogError tests that stopping a server doesn't log an error +// when the listener is closed as expected +func TestServerStopDoesNotLogError(t *testing.T) { + req := require.New(t) + + // Create a server with a test driver + server := NewFtpServer(&TestServerDriver{ + Settings: &Settings{ + ListenAddr: "127.0.0.1:0", // Use dynamic port + }, + }) + + // Use a custom logger that tracks error logs + mockLogger := &MockLogger{} + server.Logger = mockLogger + + // Start listening + err := server.Listen() + req.NoError(err) + + // Start serving in a goroutine + var serveErr error + var waitGroup sync.WaitGroup + waitGroup.Add(1) + + go func() { + defer waitGroup.Done() + serveErr = server.Serve() + }() + + // Give the server a moment to start accepting connections + time.Sleep(100 * time.Millisecond) + + // Stop the server + err = server.Stop() + req.NoError(err) + + // Wait for the Serve goroutine to finish + waitGroup.Wait() + + // Serve should return nil (no error) when stopped normally + req.NoError(serveErr) + + // Check that no error was logged for the "use of closed network connection" + // The mock logger should not have received any error logs + req.Empty(mockLogger.ErrorLogs, "Expected no error logs when stopping server, but got: %v", mockLogger.ErrorLogs) +} + +// MockLogger captures log calls to verify behavior +type MockLogger struct { + ErrorLogs []string + WarnLogs []string + InfoLogs []string + DebugLogs []string +} + +func (m *MockLogger) Debug(message string, _ ...any) { + m.DebugLogs = append(m.DebugLogs, message) +} + +func (m *MockLogger) Info(message string, _ ...any) { + m.InfoLogs = append(m.InfoLogs, message) +} + +func (m *MockLogger) Warn(message string, _ ...any) { + m.WarnLogs = append(m.WarnLogs, message) +} + +func (m *MockLogger) Error(message string, _ ...any) { + m.ErrorLogs = append(m.ErrorLogs, message) +} + +func (m *MockLogger) Panic(message string, _ ...any) { + panic(message) +} + +func (m *MockLogger) With(_ ...any) log.Logger { + return m +} diff --git a/server_test.go b/server_test.go index 35c2415a..98afb881 100644 --- a/server_test.go +++ b/server_test.go @@ -84,7 +84,8 @@ func newFakeListener(err error) net.Listener { func TestCannotListen(t *testing.T) { req := require.New(t) - portBlockerListener, err := net.Listen("tcp", "127.0.0.1:0") + lc := &net.ListenConfig{} + portBlockerListener, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0") req.NoError(err) defer func() { req.NoError(portBlockerListener.Close()) }() @@ -107,7 +108,8 @@ func TestCannotListen(t *testing.T) { func TestListenWithBadTLSSettings(t *testing.T) { req := require.New(t) - portBlockerListener, err := net.Listen("tcp", "127.0.0.1:0") + lc := &net.ListenConfig{} + portBlockerListener, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0") req.NoError(err) defer func() { req.NoError(portBlockerListener.Close()) }() @@ -173,7 +175,6 @@ func TestQuoteDoubling(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { require.Equal(t, tt.want, quoteDoubling(tt.args.s)) }) diff --git a/transfer_active_test.go b/transfer_active_test.go index 6f60164b..7e130cce 100644 --- a/transfer_active_test.go +++ b/transfer_active_test.go @@ -26,7 +26,8 @@ func TestRemoteAddrFormat(t *testing.T) { } func TestActiveTransferFromPort20(t *testing.T) { - listener, err := net.Listen("tcp", ":20") //nolint:gosec + lc := &net.ListenConfig{} + listener, err := lc.Listen(t.Context(), "tcp", ":20") if err != nil { t.Skipf("Binding on port 20 is not supported here: %v", err) } diff --git a/transfer_test.go b/transfer_test.go index f875cc7d..d2e1448c 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -1104,7 +1104,8 @@ func TestPASVIPMatch(t *testing.T) { server := NewTestServer(t, false) - conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second) + dialer := &net.Dialer{Timeout: 5 * time.Second} + conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr()) require.NoError(t, err) defer func() { @@ -1187,7 +1188,7 @@ func TestPassivePortExhaustion(t *testing.T) { defer func() { require.NoError(t, raw.Close()) }() - for i := 0; i < 20; i++ { + for range 20 { rc, message, err := raw.SendCommand("PASV") require.NoError(t, err) require.Equal(t, StatusEnteringPASV, rc, message)