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 0e906bc0e254a7548dd801644a394084723d4cbf..155d0ab04ab29e5be1ebee30a852c96c268be44a 100644 (file)
--- 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 f454214801083542500ecdc3e0f3716589a7a9fe..2ddaecbf49acf421f9d0edc380f77763712ff871 100644 (file)
--- 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.