From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Wed, 19 Aug 2015 10:18:02 +0000 (+0300)
Subject: FtpServer.cc:1024: "reply != NULL" assertion
X-Git-Tag: SQUID_4_0_1~126
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0d253dfaed93ac3bf31a2fa72d28ca457628fe2d;p=thirdparty%2Fsquid.git

FtpServer.cc:1024: "reply != NULL" assertion

Handle nil HttpReply pointer inside various handlers called from
Ftp::Server::handleReply(). For example, when the related StoreEntry
object is aborted, the client_side_reply.cc code may call the
Ftp::Server::handleReply() method with a nil reply pointer.

The Ftp::Server::handleReply() methods itself cannot handle nil replies
because they are valid in many states. Only state-specific handlers know
whether they need the reply.

The Ftp::Server::handleReply() method is called [via Store] from Client code.
Thus, exceptions in handleReply() are handled by the Ftp::Client job. That job
does not have enough information to know whether the client-to-Squid connection
should be closed; the job keeps the connection open. When the reply is nil,
that open connection becomes unusable, leading to more problems.

This patch fixes the Ftp::Server::handleReply() to handle exceptions,
including closing the connections in the case of an exception. It also
adds Must(reply) checks to check for nil HttpReply pointers where the
reply is required. Eventually, Store should start using async calls to
protect jobs waiting for Store updates. Meanwhile, this should help.

This is a Measurement Factory project.
---

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 0e906bc0e2..155d0ab04a 100644
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -785,11 +785,16 @@ Ftp::Server::handleReply(HttpReply *reply, StoreIOBuffer data)
         NULL, // fssHandleCdup
         &Ftp::Server::handleErrorReply // fssError
     };
-    const Server &server = dynamic_cast<const Ftp::Server&>(*context->getConn());
-    if (const ReplyHandler handler = handlers[server.master->serverState])
-        (this->*handler)(reply, data);
-    else
-        writeForwardedReply(reply);
+    try {
+        const Server &server = dynamic_cast<const Ftp::Server&>(*context->getConn());
+        if (const ReplyHandler handler = handlers[server.master->serverState])
+            (this->*handler)(reply, data);
+        else
+            writeForwardedReply(reply);
+    } catch (const std::exception &e) {
+        callException(e);
+        throw TexcHere(e.what());
+    }
 }
 
 void
@@ -800,6 +805,7 @@ Ftp::Server::handleFeatReply(const HttpReply *reply, StoreIOBuffer)
         return;
     }
 
+    Must(reply);
     HttpReply::Pointer featReply = Ftp::HttpReplyWrapper(211, "End", Http::scNoContent, 0);
     HttpHeader const &serverReplyHeader = reply->header;
 
@@ -1021,7 +1027,8 @@ Ftp::Server::handleUploadReply(const HttpReply *reply, StoreIOBuffer)
 void
 Ftp::Server::writeForwardedReply(const HttpReply *reply)
 {
-    assert(reply != NULL);
+    Must(reply);
+
     const HttpHeader &header = reply->header;
     // adaptation and forwarding errors lack Http::HdrType::FTP_STATUS
     if (!header.has(Http::HdrType::FTP_STATUS)) {
@@ -1102,7 +1109,7 @@ Ftp::Server::writeErrorReply(const HttpReply *reply, const int scode)
     }
 #endif
 
-    assert(reply != NULL);
+    Must(reply);
     const char *reason = reply->header.has(Http::HdrType::FTP_REASON) ?
                          reply->header.getStr(Http::HdrType::FTP_REASON):
                          reply->sline.reason();
@@ -1693,6 +1700,17 @@ Ftp::Server::setReply(const int code, const char *msg)
     http->storeEntry()->replaceHttpReply(reply);
 }
 
+void
+Ftp::Server::callException(const std::exception &e)
+{
+    debugs(33, 2, "FTP::Server job caught: " << e.what());
+    closeDataConnection();
+    unpinConnection(true);
+    if (Comm::IsConnOpen(clientConnection))
+        clientConnection->close();
+    AsyncJob::callException(e);
+}
+
 /// Whether Squid FTP Relay supports a named feature (e.g., a command).
 static bool
 Ftp::SupportedCommand(const SBuf &name)
diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h
index f454214801..2ddaecbf49 100644
--- a/src/servers/FtpServer.h
+++ b/src/servers/FtpServer.h
@@ -57,6 +57,8 @@ class Server: public ConnStateData
 public:
     explicit Server(const MasterXaction::Pointer &xact);
     virtual ~Server();
+    /* AsyncJob API */
+    virtual void callException(const std::exception &e) override;
 
     // This is a pointer in hope to minimize future changes when MasterState
     // becomes a part of MasterXaction. Guaranteed not to be nil.