To minimize TCP races, delay complaining about missing passive data connection

to Squid until we talk to the server.

After PASV, it is possible for the client TCP handshake to reach Squid _after_
a LIST or RETR command is parsed, even if the client properly initiates the
connection before writing the command. We no longer immediately respond with
425 "Data connection is not established" in such cases, but re-check the data
connection availability after we talk to the server.

Also polished/fixed a few comments.

diff --git a/src/client_side.cc b/src/client_side.cc
index c2d6cd52f3532a134c71e91a2d32533c1064fb78..648fbcd780221ea2d00fb76df7caa4f8af133a4b 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -268,7 +268,8 @@ static FtpRequestHandler FtpHandlePortRequest;
 static FtpRequestHandler FtpHandleDataRequest;
 static FtpRequestHandler FtpHandleUploadRequest;
 
-static bool FtpCheckDataConnection(ClientSocketContext *context);
+static bool FtpCheckDataConnPre(ClientSocketContext *context);
+static bool FtpCheckDataConnPost(ClientSocketContext *context);
 static void FtpSetReply(ClientSocketContext *context, const int code, const char *msg);
 static bool FtpSupportedCommand(const String &name);
 
@@ -5162,7 +5163,7 @@ FtpHandleReply(ClientSocketContext *context, HttpReply *reply, StoreIOBuffer dat
         FtpHandlePasvReply, // FTP_HANDLE_PASV
         FtpHandlePortReply, // FTP_HANDLE_PORT
         FtpHandleDataReply, // FTP_HANDLE_DATA_REQUEST
-        FtpHandleUploadReply, // FTP_HANDLE_DATA_REQUEST
+        FtpHandleUploadReply, // FTP_HANDLE_UPLOAD_REQUEST
         FtpHandleErrorReply // FTP_ERROR
     };
     const ConnStateData::FtpState state = context->getConn()->ftp.state;
@@ -5315,6 +5316,15 @@ FtpHandleDataReply(ClientSocketContext *context, const HttpReply *reply, StoreIO
         return;
     }
 
+    if (!conn->ftp.dataConn) {
+        // We got STREAM_COMPLETE (or error) and closed the client data conn.
+        debugs(33, 3, "ignoring FTP srv data response after clt data closure");
+        return;
+    }
+
+    if (!FtpCheckDataConnPost(context))
+        return;
+
     debugs(33, 7, HERE << data.length);
 
     if (data.length <= 0) {
@@ -5322,12 +5332,6 @@ FtpHandleDataReply(ClientSocketContext *context, const HttpReply *reply, StoreIO
         return;
     }
 
-    if (!Comm::IsConnOpen(conn->ftp.dataConn)) {
-        debugs(33, 3, HERE << "got FTP reply data when client data connection "
-               "is closed, ignoring");
-        return;
-    }
-
     MemBuf mb;
     mb.init(data.length + 1, data.length + 1);
     mb.append(data.data, data.length);
@@ -5386,6 +5390,9 @@ FtpWroteReplyData(const Comm::ConnectionPointer &conn, char *bufnotused, size_t
 static void
 FtpHandleUploadReply(ClientSocketContext *context, const HttpReply *reply, StoreIOBuffer data)
 {
+    if (!FtpCheckDataConnPost(context))
+        return;
+  
     FtpWriteForwardedReply(context, reply);
 }
 
@@ -5727,7 +5734,7 @@ FtpHandlePortRequest(ClientSocketContext *context, String &cmd, String &params)
 bool
 FtpHandleDataRequest(ClientSocketContext *context, String &cmd, String &params)
 {
-    if (!FtpCheckDataConnection(context))
+    if (!FtpCheckDataConnPre(context))
         return false;
 
     FtpChangeState(context->getConn(), ConnStateData::FTP_HANDLE_DATA_REQUEST, "FtpHandleDataRequest");
@@ -5738,7 +5745,7 @@ FtpHandleDataRequest(ClientSocketContext *context, String &cmd, String &params)
 bool
 FtpHandleUploadRequest(ClientSocketContext *context, String &cmd, String &params)
 {
-    if (!FtpCheckDataConnection(context))
+    if (!FtpCheckDataConnPre(context))
         return false;
 
     FtpChangeState(context->getConn(), ConnStateData::FTP_HANDLE_UPLOAD_REQUEST, "FtpHandleDataRequest");
@@ -5746,20 +5753,26 @@ FtpHandleUploadRequest(ClientSocketContext *context, String &cmd, String &params
     return true;
 }
 
+/// check that client data connection is ready for future I/O or at least
+/// has a chance of becoming ready soon.
 bool
-FtpCheckDataConnection(ClientSocketContext *context)
+FtpCheckDataConnPre(ClientSocketContext *context)
 {
     ConnStateData *const connState = context->getConn();
     if (Comm::IsConnOpen(connState->ftp.dataConn))
         return true;
 
     if (Comm::IsConnOpen(connState->ftp.dataListenConn)) {
-        FtpSetReply(context, 425, "Data connection is not established");
-        return false;
+        // We are still waiting for a client to connect to us after PASV.
+        // Perhaps client's data conn handshake has not reached us yet.
+        // After we talk to the server, FtpCheckDataConnPost() will recheck.
+        debugs(33, 3, "expecting clt data conn " << connState->ftp.dataListenConn);
+        return true;
     }
 
     if (!connState->ftp.dataConn || connState->ftp.dataConn->remote.isAnyAddr()) {
-        // XXX: use client address and default port instead.
+        debugs(33, 5, "missing " << connState->ftp.dataConn);
+        // TODO: use client address and default port instead.
         FtpSetReply(context, 425, "Use PORT or PASV first");
         return false;
     }
@@ -5776,6 +5789,23 @@ FtpCheckDataConnection(ClientSocketContext *context)
     return false; // ConnStateData::processFtpRequest waits FtpHandleConnectDone
 }
 
+/// Check that client data connection is ready for immediate I/O.
+static bool
+FtpCheckDataConnPost(ClientSocketContext *context)
+{
+    const ConnStateData *connState = context->getConn();
+    assert(connState);
+    const Comm::ConnectionPointer &dataConn = connState->ftp.dataConn;
+    if (dataConn != NULL && !Comm::IsConnOpen(dataConn)) {
+        // This check is deliberately missing from FtpCheckDataConnPre()
+        debugs(33, 3, "missing client data conn: " << dataConn);
+        FtpWriteCustomReply(context, 425, "Data connection is not established");
+        dataConn->close();
+        return false;
+    }
+    return true;
+}
+
 void
 FtpHandleConnectDone(const Comm::ConnectionPointer &conn, comm_err_t status, int xerrno, void *data)
 {