From: Christos Tsantilas Date: Tue, 15 Mar 2016 12:43:09 +0000 (+0200) Subject: assertion failed: Write.cc:41: "!ccb->active()" X-Git-Tag: SQUID_4_0_8~32 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3238b9b66f062e9b7ef3bb7387d8276db1b0576b;p=thirdparty%2Fsquid.git assertion failed: Write.cc:41: "!ccb->active()" 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 --- diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc index 6fb87c0669..5bba7381af 100644 --- a/src/clients/FtpRelay.cc +++ b/src/clients/FtpRelay.cc @@ -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 &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 &mgr = fwd->request->clientConnectionManager; + if (mgr.valid()) { + if (Ftp::Server *srv = dynamic_cast(mgr.get())) { + typedef NullaryMemFunT 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 &mgr = fwd->request->clientConnectionManager; - if (mgr.valid()) { - if (Ftp::Server *srv = dynamic_cast(mgr.get())) { - typedef NullaryMemFunT 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 &mgr = fwd->request->clientConnectionManager; + if (mgr.valid()) { + if (Ftp::Server *srv = dynamic_cast(mgr.get())) { + typedef UnaryMemFunT 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) { diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 1aa6a09dcb..7409fd8a9a 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -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(); diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h index 15fc37f6d8..aaf8d6e687 100644 --- a/src/servers/FtpServer.h +++ b/src/servers/FtpServer.h @@ -41,7 +41,7 @@ class MasterState: public RefCountable public: typedef RefCount 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