------------------------------------------------------------ revno: 14001 revision-id: chtsanti@users.sourceforge.net-20160315173314-axlaq4gt0w3q037u parent: chtsanti@users.sourceforge.net-20160302091210-z6na5qq51cknovnd committer: Christos Tsantilas branch nick: 3.5 timestamp: Tue 2016-03-15 19:33:14 +0200 message: 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 ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: chtsanti@users.sourceforge.net-20160315173314-\ # axlaq4gt0w3q037u # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: fa70483a3b994486dd360cf2dff75f409fbc2cef # timestamp: 2016-03-15 17:50:57 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: chtsanti@users.sourceforge.net-20160302091210-\ # z6na5qq51cknovnd # # Begin patch === modified file 'src/clients/FtpRelay.cc' --- src/clients/FtpRelay.cc 2016-01-31 06:14:05 +0000 +++ src/clients/FtpRelay.cc 2016-03-15 17:33:14 +0000 @@ -56,6 +56,7 @@ /* AsyncJob API */ virtual void start(); + virtual void swanSong(); void forwardReply(); void forwardError(err_type error = ERR_NONE, int xerrno = 0); @@ -87,12 +88,18 @@ 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 @@ -142,7 +149,8 @@ AsyncJob("Ftp::Relay"), Ftp::Client(fwdState), thePreliminaryCb(NULL), - forwardingCompleted(false) + forwardingCompleted(false), + originWaitInProgress(false) { savedReply.message = NULL; savedReply.lastCommand = NULL; @@ -178,11 +186,20 @@ 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)) { @@ -527,6 +544,19 @@ 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 @@ -683,7 +713,9 @@ " after reading response data"); } - finalizeDataDownload(); + debugs(9, 2, "Complete data downloading"); + + serverComplete(); } void @@ -711,25 +743,6 @@ 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) { @@ -750,6 +763,23 @@ } 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) { Ftp::Relay *ftpClient = (Ftp::Relay *)d; === modified file 'src/servers/FtpServer.cc' --- src/servers/FtpServer.cc 2016-01-31 06:14:05 +0000 +++ src/servers/FtpServer.cc 2016-03-15 17:33:14 +0000 @@ -59,7 +59,9 @@ uploadAvailSize(0), listener(), connector(), - reader() + reader(), + waitingForOrigin(false), + originDataDownloadAbortedOnError(false) { flags.readMore = false; // we need to announce ourselves first *uploadBuf = 0; @@ -1033,6 +1035,12 @@ { Must(reply); + if (waitingForOrigin) { + Must(delayedReply == NULL); + delayedReply = reply; + return; + } + const HttpHeader &header = reply->header; // adaptation and forwarding errors lack HDR_FTP_STATUS if (!header.has(HDR_FTP_STATUS)) { @@ -1489,8 +1497,8 @@ if (!checkDataConnPre()) return false; - master->waitForOriginData = true; master->userDataDone = 0; + originDataDownloadAbortedOnError = false; changeState(fssHandleDataRequest, "handleDataRequest"); @@ -1503,9 +1511,6 @@ if (!checkDataConnPre()) return false; - master->waitForOriginData = true; - master->userDataDone = 0; - changeState(fssHandleUploadRequest, "handleDataRequest"); return true; @@ -1705,14 +1710,46 @@ } void -Ftp::Server::originDataCompletionCheckpoint() -{ - if (!master->userDataDone) { - debugs(33, 5, "Transfering from/to client not finished yet"); +Ftp::Server::startWaitingForOrigin() +{ + 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 != nullptr) { + HttpReply::Pointer reply = delayedReply; + delayedReply = nullptr; + writeForwardedReply(reply.getRaw()); + return; // do not completeDataDownload() after an earlier response + } + + 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); } - - completeDataExchange(); } void Ftp::Server::userDataCompletionCheckpoint(int finalStatusCode) @@ -1723,23 +1760,24 @@ if (in.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(); === modified file 'src/servers/FtpServer.h' --- src/servers/FtpServer.h 2016-01-31 05:39:09 +0000 +++ src/servers/FtpServer.h 2016-03-15 17:33:14 +0000 @@ -41,7 +41,7 @@ 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 @@ 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. @@ -62,10 +60,14 @@ /* AsyncJob API */ virtual void callException(const std::exception &e); + /// 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. @@ -121,7 +123,7 @@ /// 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); @@ -186,6 +188,14 @@ 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; + CBDATA_CLASS2(Server); };