]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
assertion failed: Write.cc:41: "!ccb->active()"
authorChristos Tsantilas <chtsanti@users.sourceforge.net>
Tue, 15 Mar 2016 12:43:09 +0000 (14:43 +0200)
committerChristos Tsantilas <chtsanti@users.sourceforge.net>
Tue, 15 Mar 2016 12:43:09 +0000 (14:43 +0200)
Bug description:
   - The client side and server side are finished
   - On server side the Ftp::Relay::finalizeDataDownload() is called and
     schedules the Ftp::Server::originDataCompletionCheckpoint
   - On client side the "Ftp::Server::userDataCompletionCheckpoint" is
     called. This is schedules a write to control connection and closes
     data connection.
   - The Ftp::Server::originDataCompletionCheckpoint is called which is
     trying to write to control connection and the assertion triggered.

This bug is an corner case, where the client-side  (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job before
respond to the FTP client. In this bug the existing mechanism, designed
to handle such problems, did not worked correctly and resulted to a double
write response to the client.

This patch try to fix the existing mechanism as follows:

- When Ftp::Server receives a "startWaitingForOrigin" callback, postpones
  writting possible responses to the client and keeps waiting for the
  stopWaitingForOrigin callback

- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
  resumes any postponed response.

- When the Ftp::Client starts working on a DATA-related transaction, calls the
  Ftp::Server::startWaitingForOrigin callback

- When the Ftp::Client finishes its job or when its abort abnormaly, checks
  whether it needs to call Ftp::Server::stopWaitingForOrigin callback.

- Also this patch try to fix the status code returned to the FTP client
  taking in account the status code returned by FTP server. The
  "Ftp::Server::stopWaitingForOrigin" is used to pass the returned status code
  to the client side.

This is a Measurement Factory project

src/clients/FtpRelay.cc
src/servers/FtpServer.cc
src/servers/FtpServer.h

index 6fb87c06696fb42fccec6b7cc729231677509b6d..5bba7381afb3f3762ccd8e94dd153c772cc0a8ab 100644 (file)
@@ -59,6 +59,7 @@ protected:
 
     /* AsyncJob API */
     virtual void start();
+    virtual void swanSong();
 
     void forwardReply();
     void forwardError(err_type error = ERR_NONE, int xerrno = 0);
@@ -90,12 +91,18 @@ protected:
     void readUserOrPassReply();
 
     void scheduleReadControlReply();
-    void finalizeDataDownload();
+
+    /// Inform Ftp::Server that we are done if originWaitInProgress
+    void stopOriginWait(int code);
 
     static void abort(void *d); // TODO: Capitalize this and FwdState::abort().
 
     bool forwardingCompleted; ///< completeForwarding() has been called
 
+    /// whether we are between Ftp::Server::startWaitingForOrigin() and
+    /// Ftp::Server::stopWaitingForOrigin() calls
+    bool originWaitInProgress;
+
     struct {
         wordlist *message; ///< reply message, one  wordlist entry per message line
         char *lastCommand; ///< the command caused the reply
@@ -143,7 +150,8 @@ Ftp::Relay::Relay(FwdState *const fwdState):
     AsyncJob("Ftp::Relay"),
     Ftp::Client(fwdState),
     thePreliminaryCb(NULL),
-    forwardingCompleted(false)
+    forwardingCompleted(false),
+    originWaitInProgress(false)
 {
     savedReply.message = NULL;
     savedReply.lastCommand = NULL;
@@ -179,11 +187,20 @@ Ftp::Relay::start()
         sendCommand();
 }
 
+void
+Ftp::Relay::swanSong()
+{
+    stopOriginWait(0);
+    Ftp::Client::swanSong();
+}
+
 /// Keep control connection for future requests, after we are done with it.
 /// Similar to COMPLETE_PERSISTENT_MSG handling in http.cc.
 void
 Ftp::Relay::serverComplete()
 {
+    stopOriginWait(ctrl.replycode);
+
     CbcPointer<ConnStateData> &mgr = fwd->request->clientConnectionManager;
     if (mgr.valid()) {
         if (Comm::IsConnOpen(ctrl.conn)) {
@@ -531,6 +548,19 @@ Ftp::Relay::sendCommand()
         serverState() == fssConnected ? SENT_USER :
         serverState() == fssHandlePass ? SENT_PASS :
         SENT_COMMAND;
+
+    if (state == SENT_DATA_REQUEST) {
+        CbcPointer<ConnStateData> &mgr = fwd->request->clientConnectionManager;
+        if (mgr.valid()) {
+            if (Ftp::Server *srv = dynamic_cast<Ftp::Server*>(mgr.get())) {
+                typedef NullaryMemFunT<Ftp::Server> CbDialer;
+                AsyncCall::Pointer call = JobCallback(11, 3, CbDialer, srv,
+                                                      Ftp::Server::startWaitingForOrigin);
+                ScheduleCallHere(call);
+                originWaitInProgress = true;
+            }
+        }
+    }
 }
 
 void
@@ -689,7 +719,9 @@ Ftp::Relay::readTransferDoneReply()
                " after reading response data");
     }
 
-    finalizeDataDownload();
+    debugs(9, 2, "Complete data downloading");
+
+    serverComplete();
 }
 
 void
@@ -717,25 +749,6 @@ Ftp::Relay::scheduleReadControlReply()
     Ftp::Client::scheduleReadControlReply(0);
 }
 
-void
-Ftp::Relay::finalizeDataDownload()
-{
-    debugs(9, 2, "Complete data downloading/Uploading");
-
-    updateMaster().waitForOriginData = false;
-
-    CbcPointer<ConnStateData> &mgr = fwd->request->clientConnectionManager;
-    if (mgr.valid()) {
-        if (Ftp::Server *srv = dynamic_cast<Ftp::Server*>(mgr.get())) {
-            typedef NullaryMemFunT<Ftp::Server> CbDialer;
-            AsyncCall::Pointer call = JobCallback(11, 3, CbDialer, srv,
-                                                  Ftp::Server::originDataCompletionCheckpoint);
-            ScheduleCallHere(call);
-        }
-    }
-    serverComplete();
-}
-
 bool
 Ftp::Relay::abortOnData(const char *reason)
 {
@@ -755,6 +768,23 @@ Ftp::Relay::abortOnData(const char *reason)
     return !Comm::IsConnOpen(ctrl.conn);
 }
 
+void
+Ftp::Relay::stopOriginWait(int code)
+{
+    if (originWaitInProgress) {
+        CbcPointer<ConnStateData> &mgr = fwd->request->clientConnectionManager;
+        if (mgr.valid()) {
+            if (Ftp::Server *srv = dynamic_cast<Ftp::Server*>(mgr.get())) {
+                typedef UnaryMemFunT<Ftp::Server, int> CbDialer;
+                AsyncCall::Pointer call = asyncCall(11, 3, "Ftp::Server::stopWaitingForOrigin",
+                                                    CbDialer(srv, &Ftp::Server::stopWaitingForOrigin, code));
+                ScheduleCallHere(call);
+            }
+        }
+        originWaitInProgress = false;
+    }
+}
+
 void
 Ftp::Relay::abort(void *d)
 {
index 1aa6a09dcb741e344aebce421eca0ff80db58df0..7409fd8a9a6e2850a890509694d4efe3e3063182 100644 (file)
@@ -62,7 +62,9 @@ Ftp::Server::Server(const MasterXaction::Pointer &xact):
     uploadAvailSize(0),
     listener(),
     connector(),
-    reader()
+    reader(),
+    waitingForOrigin(false),
+    originDataDownloadAbortedOnError(false)
 {
     flags.readMore = false; // we need to announce ourselves first
     *uploadBuf = 0;
@@ -1034,6 +1036,12 @@ Ftp::Server::writeForwardedReply(const HttpReply *reply)
 {
     Must(reply);
 
+    if (waitingForOrigin) {
+        Must(delayedReply == NULL);
+        delayedReply = reply;
+        return;
+    }
+
     const HttpHeader &header = reply->header;
     // adaptation and forwarding errors lack Http::HdrType::FTP_STATUS
     if (!header.has(Http::HdrType::FTP_STATUS)) {
@@ -1488,8 +1496,8 @@ Ftp::Server::handleDataRequest(String &, String &)
     if (!checkDataConnPre())
         return false;
 
-    master->waitForOriginData = true;
     master->userDataDone = 0;
+    originDataDownloadAbortedOnError = false;
 
     changeState(fssHandleDataRequest, "handleDataRequest");
 
@@ -1502,9 +1510,6 @@ Ftp::Server::handleUploadRequest(String &, String &)
     if (!checkDataConnPre())
         return false;
 
-    master->waitForOriginData = true;
-    master->userDataDone = 0;
-
     if (Config.accessList.forceRequestBodyContinuation) {
         ClientHttpRequest *http = pipeline.front()->http;
         HttpRequest *request = http->request;
@@ -1724,14 +1729,46 @@ Ftp::Server::callException(const std::exception &e)
 }
 
 void
-Ftp::Server::originDataCompletionCheckpoint()
+Ftp::Server::startWaitingForOrigin()
 {
-    if (!master->userDataDone) {
-        debugs(33, 5, "Transfering from/to client not finished yet");
-        return;
+    debugs(33, 5, "waiting for Ftp::Client data transfer to end");
+    waitingForOrigin = true;
+}
+
+void
+Ftp::Server::stopWaitingForOrigin(int originStatus)
+{
+    Must(waitingForOrigin);
+    waitingForOrigin = false;
+
+    // if we have already decided how to respond, respond now
+    if (delayedReply) {
+        HttpReply::Pointer reply = delayedReply;
+        delayedReply = nullptr;
+        writeForwardedReply(reply.getRaw());
+        return; // do not completeDataDownload() after an earlier response
     }
 
-    completeDataExchange();
+    if (master->serverState != fssHandleDataRequest)
+        return;
+
+    // completeDataDownload() could be waitingForOrigin in fssHandleDataRequest
+    // Depending on which side has finished downloading first, either trust 
+    // master->userDataDone status or set originDataDownloadAbortedOnError:
+    if (master->userDataDone) {
+        // We finished downloading before Ftp::Client. Most likely, the
+        // adaptation shortened the origin response or we hit an error.
+        // Our status (stored in master->userDataDone) is more informative.
+        // Use master->userDataDone; avoid originDataDownloadAbortedOnError.
+        completeDataDownload();
+    } else {
+        debugs(33, 5, "too early to write the response");
+        // Ftp::Client naturally finished downloading before us. Set
+        // originDataDownloadAbortedOnError to overwrite future
+        // master->userDataDone and relay Ftp::Client error, if there was
+        // any, to the user.
+        originDataDownloadAbortedOnError = (originStatus >= 400);
+    }
 }
 
 void Ftp::Server::userDataCompletionCheckpoint(int finalStatusCode)
@@ -1742,23 +1779,24 @@ void Ftp::Server::userDataCompletionCheckpoint(int finalStatusCode)
     if (bodyParser)
         finishDechunkingRequest(false);
 
-    // The origin control connection is gone, nothing to wait for
-    if (!Comm::IsConnOpen(pinning.serverConnection))
-        master->waitForOriginData = false;
-
-    if (master->waitForOriginData) {
-        // The completeDataExchange() is not called here unconditionally
+    if (waitingForOrigin) {
+        // The completeDataDownload() is not called here unconditionally
         // because we want to signal the FTP user that we are not fully
         // done processing its data stream, even though all data bytes
         // have been sent or received already.
-        debugs(33, 5, "Transfering from/to FTP server is not complete");
+        debugs(33, 5, "Transfering from FTP server is not complete");
         return;
     }
 
-    completeDataExchange();
+    // Adjust our reply if the server aborted with an error before we are done.
+    if (master->userDataDone == 226 && originDataDownloadAbortedOnError) {
+        debugs(33, 5, "Transfering from FTP server terminated with an error, adjust status code");
+        master->userDataDone = 451;
+    }
+    completeDataDownload();
 }
 
-void Ftp::Server::completeDataExchange()
+void Ftp::Server::completeDataDownload()
 {
     writeCustomReply(master->userDataDone, master->userDataDone == 226 ? "Transfer complete" : "Server error; transfer aborted");
     closeDataConnection();
index 15fc37f6d8bb2bd60015be5de221030a9dbc531e..aaf8d6e687bd2fadce324cea7f33c458c6f7122f 100644 (file)
@@ -41,7 +41,7 @@ class MasterState: public RefCountable
 public:
     typedef RefCount<MasterState> Pointer;
 
-    MasterState(): serverState(fssBegin), clientReadGreeting(false), userDataDone(0), waitForOriginData(false) {}
+MasterState(): serverState(fssBegin), clientReadGreeting(false), userDataDone(0) {}
 
     Ip::Address clientDataAddr; ///< address of our FTP client data connection
     SBuf workingDir; ///< estimated current working directory for URI formation
@@ -49,8 +49,6 @@ public:
     bool clientReadGreeting; ///< whether our FTP client read their FTP server greeting
     /// Squid will send or has sent this final status code to the FTP client
     int userDataDone;
-    /// whether the transfer on the Squid-origin data connection is not over yet
-    bool waitForOriginData;
 };
 
 /// Manages a control connection from an FTP client.
@@ -65,10 +63,14 @@ public:
     /* AsyncJob API */
     virtual void callException(const std::exception &e) override;
 
+    /// Called by Ftp::Client class when it is start receiving or
+    /// sending data.
+    void startWaitingForOrigin();
+
     /// Called by Ftp::Client class when it is done receiving or
     /// sending data. Waits for both agents to be done before
     /// responding to the FTP client and closing the data connection.
-    void originDataCompletionCheckpoint();
+    void stopWaitingForOrigin(int status);
 
     // This is a pointer in hope to minimize future changes when MasterState
     // becomes a part of MasterXaction. Guaranteed not to be nil.
@@ -124,7 +126,7 @@ protected:
 
     /// Writes the data-transfer status reply to the FTP client and
     /// closes the data connection.
-    void completeDataExchange();
+    void completeDataDownload();
 
     void calcUri(const SBuf *file);
     void changeState(const Ftp::ServerState newState, const char *reason);
@@ -188,6 +190,14 @@ private:
     AsyncCall::Pointer listener; ///< set when we are passively listening
     AsyncCall::Pointer connector; ///< set when we are actively connecting
     AsyncCall::Pointer reader; ///< set when we are reading FTP data
+
+    /// whether we wait for the origin data transfer to end
+    bool waitingForOrigin;
+    /// whether the origin data transfer aborted
+    bool originDataDownloadAbortedOnError;
+    
+    /// a response which writing was postponed until stopWaitingForOrigin()
+    HttpReply::Pointer delayedReply;
 };
 
 } // namespace Ftp