diff --git a/client_handler.go b/client_handler.go index 54c7d78e..b6656b1a 100644 --- a/client_handler.go +++ b/client_handler.go @@ -455,9 +455,9 @@ func (c *clientHandler) readCommand() bool { } if err != nil { - c.handleCommandsStreamError(err) + shouldDisconnect := c.handleCommandsStreamError(err) - return true + return shouldDisconnect } line := string(lineSlice) @@ -471,11 +471,31 @@ func (c *clientHandler) readCommand() bool { return false } -func (c *clientHandler) handleCommandsStreamError(err error) { +func (c *clientHandler) handleCommandsStreamError(err error) bool { // florent(2018-01-14): #58: IDLE timeout: Adding some code to deal with the deadline var errNetError net.Error if errors.As(err, &errNetError) { //nolint:nestif // too much effort to change for now if errNetError.Timeout() { + // Check if there's an active data transfer before closing the control connection + c.transferMu.Lock() + hasActiveTransfer := c.isTransferOpen + c.transferMu.Unlock() + + if hasActiveTransfer { + // If there's an active data transfer, extend the deadline and continue + extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout))) + if errSet := c.conn.SetDeadline(extendedDeadline); errSet != nil { + c.logger.Error("Could not extend read deadline during active transfer", "err", errSet) + } + + if c.debug { + c.logger.Debug("Idle timeout occurred during active transfer, extending deadline") + } + + // Don't disconnect - the transfer is still active + return false + } + // We have to extend the deadline now if errSet := c.conn.SetDeadline(time.Now().Add(time.Minute)); errSet != nil { c.logger.Error("Could not set read deadline", "err", errSet) @@ -490,19 +510,23 @@ func (c *clientHandler) handleCommandsStreamError(err error) { c.logger.Error("Flush error", "err", errFlush) } - return + return true } c.logger.Error("Network error", "err", err) - } else { - if errors.Is(err, io.EOF) { - if c.debug { - c.logger.Debug("Client disconnected", "clean", false) - } - } else { - c.logger.Error("Read error", "err", err) + + return true + } + + if errors.Is(err, io.EOF) { + if c.debug { + c.logger.Debug("Client disconnected", "clean", false) } + } else { + c.logger.Error("Read error", "err", err) } + + return true } // handleCommand takes care of executing the received line diff --git a/idle_timeout_transfer_test.go b/idle_timeout_transfer_test.go new file mode 100644 index 00000000..a8c312b9 --- /dev/null +++ b/idle_timeout_transfer_test.go @@ -0,0 +1,66 @@ +package ftpserver + +import ( + "bytes" + "testing" + "time" + + "github.com/secsy/goftp" + "github.com/stretchr/testify/require" +) + +// TestIdleTimeoutDuringTransfer verifies that the idle timeout doesn't close +// the control connection when a data transfer is active. +func TestIdleTimeoutDuringTransfer(t *testing.T) { + // Create a server with a very short idle timeout + // The test driver adds 500ms delay for files with "delay-io" in the name + server := NewTestServerWithTestDriver(t, &TestServerDriver{ + Debug: true, + Settings: &Settings{ + IdleTimeout: 1, // 1 second idle timeout + }, + }) + + conf := goftp.Config{ + User: authUser, + Password: authPass, + } + + client, err := goftp.DialConfig(conf, server.Addr()) + require.NoError(t, err, "Couldn't connect") + + defer func() { panicOnError(client.Close()) }() + + // Create test data - size chosen so that with 500ms delays per read, + // the transfer will take longer than the 1 second idle timeout + data := make([]byte, 1024*1024) // 1MB + for i := range data { + data[i] = byte(i % 256) + } + + // Upload the file with "delay-io" in the name to trigger slow I/O + // This will cause each Read() operation to take 500ms + err = client.Store("delay-io-test.bin", bytes.NewReader(data)) + require.NoError(t, err, "Failed to upload file") + + // Download the file - this will trigger multiple 500ms delays + // Total time will exceed the 1 second idle timeout + // The server should extend the deadline during the active transfer + buf := &bytes.Buffer{} + start := time.Now() + err = client.Retrieve("delay-io-test.bin", buf) + elapsed := time.Since(start) + + require.NoError(t, err, "Transfer should succeed despite idle timeout") + require.Equal(t, len(data), buf.Len(), "Downloaded data should match uploaded data") + require.Equal(t, data, buf.Bytes(), "Downloaded content should match uploaded content") + + // Verify the transfer took longer than the idle timeout + // This proves the deadline was extended during the transfer + require.Greater(t, elapsed, time.Duration(server.settings.IdleTimeout)*time.Second, + "Transfer should take longer than idle timeout to verify deadline extension worked") + + // Verify the connection is still alive after the transfer + _, err = client.ReadDir("/") + require.NoError(t, err, "Connection should still be alive after long transfer") +} diff --git a/transfer_test.go b/transfer_test.go index d2e1448c..38756076 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -1360,3 +1360,66 @@ func getPortFromEPSVResponse(t *testing.T, resp string) int { return port } + +// TestConnectionCloseDuringTransfer tests the behavior when the control connection is closed +// during an active data transfer in passive mode. This ensures proper cleanup and error handling. +func TestConnectionCloseDuringTransfer(t *testing.T) { + t.Parallel() + + server := NewTestServer(t, false) + + conf := goftp.Config{ + User: authUser, + Password: authPass, + } + + client, err := goftp.DialConfig(conf, server.Addr()) + require.NoError(t, err, "Couldn't connect") + + // Upload a file first so we have something to download + file := createTemporaryFile(t, 10*1024*1024) // 10MB file + err = client.Store("large-file.bin", file) + require.NoError(t, err, "Failed to upload file") + + // Open a raw connection to have more control + raw, err := client.OpenRawConn() + require.NoError(t, err) + + // Prepare data connection + _, err = raw.PrepareDataConn() + require.NoError(t, err) + + // Start the RETR command + returnCode, response, err := raw.SendCommand("RETR large-file.bin") + require.NoError(t, err) + require.Equal(t, StatusFileStatusOK, returnCode, response) + + // Give the transfer a moment to start + time.Sleep(50 * time.Millisecond) + + // Now close the raw connection abruptly during the transfer + // This simulates a network disconnection or client crash + err = raw.Close() + require.NoError(t, err) + + // Close the main client connection as well + _ = client.Close() + // We expect an error here since the connection is already closed/broken + // but we don't want to fail the test - this is expected behavior + + // Give the server time to clean up + time.Sleep(100 * time.Millisecond) + + // Verify we can establish a new connection and the server is still functional + newClient, err := goftp.DialConfig(conf, server.Addr()) + require.NoError(t, err, "Server should still be functional after connection close during transfer") + + defer func() { + err = newClient.Close() + require.NoError(t, err) + }() + + // Verify the file is still there and accessible + _, err = newClient.Stat("large-file.bin") + require.NoError(t, err, "File should still be accessible after interrupted transfer") +}