------------------------------------------------------------ revno: 14115 revision-id: squid3@treenet.co.nz-20161130215630-c42qucqar9bi9a1k parent: squid3@treenet.co.nz-20161130154205-c9z1bhqzuh3rafl3 fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=4004 author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Thu 2016-12-01 10:56:30 +1300 message: 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 ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20161130215630-c42qucqar9bi9a1k # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 345883c1b5a5cd221e9d0e68b254df7d955372ad # timestamp: 2016-11-30 22:42:02 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20161130154205-\ # c9z1bhqzuh3rafl3 # # Begin patch === modified file 'src/clients/FtpClient.cc' --- src/clients/FtpClient.cc 2016-08-05 14:59:33 +0000 +++ src/clients/FtpClient.cc 2016-11-30 21:56:30 +0000 @@ -442,6 +442,11 @@ 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 @@ 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 @@ 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); === modified file 'src/clients/FtpGateway.cc' --- src/clients/FtpGateway.cc 2016-01-31 05:39:09 +0000 +++ src/clients/FtpGateway.cc 2016-11-30 21:56:30 +0000 @@ -212,7 +212,9 @@ 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 @@ 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 AcceptDialer; @@ -1183,7 +1190,7 @@ 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 @@ if (ftpState->handlePasvReply(srvAddr)) ftpState->connectDataChannel(); else { - ftpSendEPRT(ftpState); + ftpFail(ftpState); + // Currently disabled, does not work correctly: + // ftpSendEPRT(ftpState); return; } } @@ -1790,6 +1799,11 @@ } 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 @@ 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 @@ ftpState->writeCommand(cbuf); ftpState->state = Ftp::Client::SENT_EPRT; } +#endif static void ftpReadEPRT(Ftp::Gateway * ftpState) @@ -1939,10 +1959,8 @@ { 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 @@ 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 @@ 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; }