Bug 4004 partial: Fix segfault via Ftp::Client::readControlReply

Added nil dereference checks for Ftp::Client::ctrl.conn, including:
- Ftp::Client::handlePasvReply() and handleEpsvReply() that dereference
  ctrl.conn in DBG_IMPORTANT messages.
- Many functions inside FtpClient.cc and FtpGateway.cc files.

TODO: We need to find a better way to handle nil ctrl.conn. It is only
a matter of time when we forget to add another dereference check or
discover a place we missed during this change.

Also disabled forwarding of EPRT and PORT commands to origin servers.
Squid support for those commands is broken and their forwarding may
cause segfaults (bug #4004). Active FTP is still supported, of course.

This is a Measurement Factory project

diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index ee95a90ed33bfcff988fc942304193a0d8250f69..3e096d4aa73c6190935384505031c27849d950d6 100644 (file)
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -442,6 +442,11 @@ Ftp::Client::handlePasvReply(Ip::Address &srvAddr)
     char *buf;
     debugs(9, 3, status());
 
+    if (!Comm::IsConnOpen(ctrl.conn)) {
+        debugs(9, 5, "The control connection to the remote end is closed");
+        return false;
+    }
+
     if (code != 227) {
         debugs(9, 2, "PASV not supported by remote end");
         return false;
@@ -473,6 +478,11 @@ Ftp::Client::handleEpsvReply(Ip::Address &remoteAddr)
     char *buf;
     debugs(9, 3, status());
 
+    if (!Comm::IsConnOpen(ctrl.conn)) {
+        debugs(9, 5, "The control connection to the remote end is closed");
+        return false;
+    }
+
     if (code != 229 && code != 522) {
         if (code == 200) {
             /* handle broken servers (RFC 2428 says OK code for EPSV MUST be 229 not 200) */
@@ -733,6 +743,11 @@ Ftp::Client::sendPassive()
 void
 Ftp::Client::connectDataChannel()
 {
+    if (!Comm::IsConnOpen(ctrl.conn)) {
+        debugs(9, 5, "The control connection to the remote end is closed");
+        return;
+    }
+
     safe_free(ctrl.last_command);
 
     safe_free(ctrl.last_reply);

diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index bda6ce2703d79dda8a4e354c6e5621eef0142d2d..5e2a88fb05c49c6ee8148c01a37289fbcb8ab421 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -212,7 +212,9 @@ static FTPSM ftpSendMdtm;
 static FTPSM ftpReadMdtm;
 static FTPSM ftpSendSize;
 static FTPSM ftpReadSize;
+#if 0
 static FTPSM ftpSendEPRT;
+#endif
 static FTPSM ftpReadEPRT;
 static FTPSM ftpSendPORT;
 static FTPSM ftpReadPORT;
@@ -450,6 +452,11 @@ Ftp::Gateway::loginParser(const char *login, int escaped)
 void
 Ftp::Gateway::listenForDataChannel(const Comm::ConnectionPointer &conn)
 {
+    if (!Comm::IsConnOpen(ctrl.conn)) {
+        debugs(9, 5, "The control connection to the remote end is closed");
+        return;
+    }
+
     assert(!Comm::IsConnOpen(data.conn));
 
     typedef CommCbMemFunT<Gateway, CommAcceptCbParams> AcceptDialer;
@@ -1183,7 +1190,7 @@ Ftp::Gateway::start()
 
     checkUrlpath();
     buildTitleUrl();
-    debugs(9, 5, HERE << "FD " << ctrl.conn->fd << " : host=" << request->GetHost() <<
+    debugs(9, 5, "FD " << (ctrl.conn != NULL ? ctrl.conn->fd : -1) << " : host=" << request->GetHost() <<
            ", path=" << request->urlpath << ", user=" << user << ", passwd=" << password);
     state = BEGIN;
     Ftp::Client::start();
@@ -1750,7 +1757,9 @@ ftpReadPasv(Ftp::Gateway * ftpState)
     if (ftpState->handlePasvReply(srvAddr))
         ftpState->connectDataChannel();
     else {
-        ftpSendEPRT(ftpState);
+        ftpFail(ftpState);
+        // Currently disabled, does not work correctly:
+        // ftpSendEPRT(ftpState);
         return;
     }
 }
@@ -1790,6 +1799,11 @@ ftpOpenListenSocket(Ftp::Gateway * ftpState, int fallback)
     }
     safe_free(ftpState->data.host);
 
+    if (!Comm::IsConnOpen(ftpState->ctrl.conn)) {
+        debugs(9, 5, "The control connection to the remote end is closed");
+        return;
+    }
+
     /*
      * Set up a listen socket on the same local address as the
      * control connection.
@@ -1875,9 +1889,14 @@ ftpReadPORT(Ftp::Gateway * ftpState)
     ftpRestOrList(ftpState);
 }
 
+#if 0
 static void
 ftpSendEPRT(Ftp::Gateway * ftpState)
 {
+    /* check the server control channel is still available */
+    if (!ftpState || !ftpState->haveControlChannel("ftpSendEPRT"))
+        return;
+
     if (Config.Ftp.epsv_all && ftpState->flags.epsv_all_sent) {
         debugs(9, DBG_IMPORTANT, "FTP does not allow EPRT method after 'EPSV ALL' has been sent.");
         return;
@@ -1913,6 +1932,7 @@ ftpSendEPRT(Ftp::Gateway * ftpState)
     ftpState->writeCommand(cbuf);
     ftpState->state = Ftp::Client::SENT_EPRT;
 }
+#endif
 
 static void
 ftpReadEPRT(Ftp::Gateway * ftpState)
@@ -1939,10 +1959,8 @@ Ftp::Gateway::ftpAcceptDataConnection(const CommAcceptCbParams &io)
 {
     debugs(9, 3, HERE);
 
-    if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
-        abortAll("entry aborted when accepting data conn");
-        data.listenConn->close();
-        data.listenConn = NULL;
+    if (!Comm::IsConnOpen(ctrl.conn)) { /*Close handlers will cleanup*/
+        debugs(9, 5, "The control connection to the remote end is closed");
         return;
     }
 
@@ -1955,6 +1973,14 @@ Ftp::Gateway::ftpAcceptDataConnection(const CommAcceptCbParams &io)
         return;
     }
 
+    if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
+        abortAll("entry aborted when accepting data conn");
+        data.listenConn->close();
+        data.listenConn = NULL;
+        io.conn->close();
+        return;
+    }
+
     /* data listening conn is no longer even open. abort. */
     if (!Comm::IsConnOpen(data.listenConn)) {
         data.listenConn = NULL; // ensure that it's cleared and not just closed.
@@ -2705,8 +2731,8 @@ void
 Ftp::Gateway::completeForwarding()
 {
     if (fwd == NULL || flags.completed_forwarding) {
-        debugs(9, 3, HERE << "completeForwarding avoids " <<
-               "double-complete on FD " << ctrl.conn->fd << ", Data FD " << data.conn->fd <<
+        debugs(9, 3, "avoid double-complete on FD " <<
+               (ctrl.conn != NULL ? ctrl.conn->fd : -1) << ", Data FD " << data.conn->fd <<
                ", this " << this << ", fwd " << fwd);
         return;
     }