From: Alex Rousskov Date: Wed, 13 Nov 2013 18:29:13 +0000 (-0700) Subject: Propagate FTP server connection closures to idle FTP gw clients but X-Git-Tag: SQUID_3_5_0_1~117^2~30 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=89b1d7a2c87a4a12cedbf9a302a223fcdd6faefe;p=thirdparty%2Fsquid.git Propagate FTP server connection closures to idle FTP gw clients but carefully close the FTP server connection upon receiving an FTP 221 response. FTP gateway code pins server connections; when there is no traffic, the client-side code holds on to the server connection. The old code did not notice pinned server connection closure until it was time to write the next FTP request to that connection. Squid now monitors the idle pinned server connection and closes the client connection if the server connection closes (or, to be more precise, if the server connection gets marked as ready for reading). Client-side monitoring of an idle pinned connection ends when the server side starts using the pinned connection again (because the server side becomes responsible for monitoring EOF conditions then). Added borrowPinnedConnection() and related methods to distinguish validation of pinned connection and responsibility transfer. Careful closure of the FTP server connection upon receiving an FTP 221 response is necessary to avoid the client-side idle server connection monitoring code closing the client connection upon detecting server-side EOF _before_ the client side had enough time to forward that 221 response to the not-yet-idle client. The latter may take some time because, in part, the response may have to go through an ICAP RESPMOD service first. --- diff --git a/src/FtpGatewayServer.cc b/src/FtpGatewayServer.cc index 1749e6076e..21cbb74f4b 100644 --- a/src/FtpGatewayServer.cc +++ b/src/FtpGatewayServer.cc @@ -133,11 +133,22 @@ ServerStateData::start() void ServerStateData::serverComplete() { - if (Comm::IsConnOpen(ctrl.conn)) { - debugs(9, 5, "preserve FTP server FD " << ctrl.conn->fd); - fwd->unregister(ctrl.conn); - ctrl.forget(); - // fwd->request->clientConnectionManager has this connection pinned + CbcPointer &mgr = fwd->request->clientConnectionManager; + if (mgr.valid()) { + if (Comm::IsConnOpen(ctrl.conn)) { + debugs(9, 7, "completing FTP server " << ctrl.conn << + " after " << ctrl.replycode); + fwd->unregister(ctrl.conn); + if (ctrl.replycode == 221) { // Server sends FTP 221 before closing + mgr->unpinConnection(false); + ctrl.close(); + } else { + mgr->pinConnection(ctrl.conn, fwd->request, + ctrl.conn->getPeer(), + fwd->request->flags.connectionAuth); + ctrl.forget(); + } + } } Ftp::ServerStateData::serverComplete(); } diff --git a/src/FwdState.cc b/src/FwdState.cc index 64f5728732..9d66a7a887 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1123,7 +1123,7 @@ FwdState::connectStart() debugs(17,7, "pinned peer connection: " << pinned_connection); // pinned_connection may become nil after a pconn race if (pinned_connection) - serverConn = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer()); + serverConn = pinned_connection->borrowPinnedConnection(request, serverDestinations[0]->getPeer()); else serverConn = NULL; if (Comm::IsConnOpen(serverConn)) { diff --git a/src/client_side.cc b/src/client_side.cc index 58ab501143..e900168bc3 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -950,7 +950,7 @@ ConnStateData::swanSong() assert(areAllContextsForThisConnection()); freeAllContexts(); - unpinConnection(); + unpinConnection(true); if (Comm::IsConnOpen(clientConnection)) clientConnection->close(); @@ -4797,7 +4797,7 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) assert(pinning.serverConnection == io.conn); pinning.closeHandler = NULL; // Comm unregisters handlers before calling const bool sawZeroReply = pinning.zeroReply; // reset when unpinning - unpinConnection(); + unpinConnection(false); if (sawZeroReply) { debugs(33, 3, "Closing client connection on pinned zero reply."); clientConnection->close(); @@ -4815,19 +4815,24 @@ ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) void ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth) { - char desc[FD_DESC_SZ]; + if (!Comm::IsConnOpen(pinning.serverConnection) || + pinning.serverConnection->fd != pinServer->fd) + pinNewConnection(pinServer, request, aPeer, auth); - if (Comm::IsConnOpen(pinning.serverConnection)) { - if (pinning.serverConnection->fd == pinServer->fd) - return; - } + startMonitoringPinnedConnection(); +} - unpinConnection(); // closes pinned connection, if any, and resets fields +void +ConnStateData::pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth) +{ + unpinConnection(true); // closes pinned connection, if any, and resets fields pinning.serverConnection = pinServer; debugs(33, 3, HERE << pinning.serverConnection); + Must(pinning.serverConnection != NULL); + // when pinning an SSL bumped connection, the request may be NULL const char *pinnedHost = "[unknown]"; if (request) { @@ -4842,6 +4847,7 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque pinning.peer = cbdataReference(aPeer); pinning.auth = auth; char stmp[MAX_IPSTRLEN]; + char desc[FD_DESC_SZ]; snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)", (auth || !aPeer) ? pinnedHost : aPeer->name, clientConnection->remote.toUrl(stmp,MAX_IPSTRLEN), @@ -4877,14 +4883,69 @@ ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *a if (!valid) { /* The pinning info is not safe, remove any pinning info */ - unpinConnection(); + unpinConnection(true); } return pinning.serverConnection; } +Comm::ConnectionPointer +ConnStateData::borrowPinnedConnection(HttpRequest *request, const CachePeer *aPeer) +{ + debugs(33, 7, pinning.serverConnection); + if (validatePinnedConnection(request, aPeer) != NULL) + stopMonitoringPinnedConnection(); + + return pinning.serverConnection; // closed if validation failed +} + +/// [re]start monitoring pinned connection for server closures so that we can +/// propagate them to an _idle_ client pinned to the server +void +ConnStateData::startMonitoringPinnedConnection() +{ + if (!pinning.reading) { + pinning.reading = true; + Comm::SetSelect(pinning.serverConnection->fd, COMM_SELECT_READ, + &ConnStateData::ReadPinnedConnection, + new Pointer(this), 0); + } +} + +/// stop or suspend monitoring pinned connection for server closures void -ConnStateData::unpinConnection() +ConnStateData::stopMonitoringPinnedConnection() +{ + if (pinning.reading) { + Comm::SetSelect(pinning.serverConnection->fd, COMM_SELECT_READ, NULL, NULL, 0); + pinning.reading = false; + } +} + +/// read callback for the idle pinned server connection +void +ConnStateData::ReadPinnedConnection(int fd, void *data) +{ + Pointer *ptr = static_cast(data); + if (ConnStateData *client = dynamic_cast(ptr->valid())) { + // get back inside job call protection + typedef NullaryMemFunT Dialer; + AsyncCall::Pointer call = JobCallback(33, 5, Dialer, client, + ConnStateData::readPinnedConnection); + ScheduleCallHere(call); + } + delete ptr; +} + +void +ConnStateData::readPinnedConnection() +{ + pinning.reading = false; // select loop clears our subscription before cb + mustStop("suspected pinned server eof"); +} + +void +ConnStateData::unpinConnection(const bool andClose) { debugs(33, 3, HERE << pinning.serverConnection); @@ -4896,9 +4957,13 @@ ConnStateData::unpinConnection() comm_remove_close_handler(pinning.serverConnection->fd, pinning.closeHandler); pinning.closeHandler = NULL; } - /// also close the server side socket, we should not use it for any future requests... - // TODO: do not close if called from our close handler? - pinning.serverConnection->close(); + + stopMonitoringPinnedConnection(); + + // close the server side socket if requested + if (andClose) + pinning.serverConnection->close(); + pinning.serverConnection = NULL; } safe_free(pinning.host); @@ -5751,7 +5816,7 @@ FtpHandleUserRequest(ConnStateData *connState, const String &cmd, String ¶ms debugs(11, 3, "reset URI from " << connState->ftp.uri << " to " << uri); FtpCloseDataConnection(connState); connState->ftp.readGreeting = false; - connState->unpinConnection(); // close control connection to the server + connState->unpinConnection(true); // close control connection to the server FtpChangeState(connState, ConnStateData::FTP_BEGIN, "URI reset"); } diff --git a/src/client_side.h b/src/client_side.h index 24fe3cad48..f29edfc160 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -267,6 +267,7 @@ public: int port; /* port of pinned connection */ bool pinned; /* this connection was pinned */ bool auth; /* pinned for www authentication */ + bool reading; ///< we are monitoring for server connection closure bool zeroReply; ///< server closed w/o response (ERR_ZERO_SIZE_OBJECT) CachePeer *peer; /* CachePeer the connection goes via */ AsyncCall::Pointer closeHandler; /*The close handler for pinned server side connection*/ @@ -297,14 +298,13 @@ public: bool handleReadData(char *buf, size_t size); bool handleRequestBodyData(); - /** - * Correlate the current ConnStateData object with the pinning_fd socket descriptor. - */ + /// forward future client requests using the given server connection + /// monitor pinned server connection for server-side closures void pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth); - /** - * Decorrelate the ConnStateData object from its pinned CachePeer - */ - void unpinConnection(); + /// undo pinConnection() and, optionally, close the pinned connection + void unpinConnection(const bool andClose); + /// returns validated pinnned server connection (and stops its monitoring) + Comm::ConnectionPointer borrowPinnedConnection(HttpRequest *request, const CachePeer *aPeer); /** * Checks if there is pinning info if it is valid. It can close the server side connection * if pinned info is not valid. @@ -423,6 +423,12 @@ private: bool concurrentRequestQueueFilled() const; + void pinNewConnection(const Comm::ConnectionPointer &pinServer, HttpRequest *request, CachePeer *aPeer, bool auth); + void startMonitoringPinnedConnection(); + void stopMonitoringPinnedConnection(); + static void ReadPinnedConnection(int fd, void *data); + void readPinnedConnection(); + #if USE_AUTH /// some user details that can be used to perform authentication on this connection Auth::UserRequest::Pointer auth_; diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc index 77d9ff4692..58d1a26e10 100644 --- a/src/tests/stub_client_side.cc +++ b/src/tests/stub_client_side.cc @@ -57,7 +57,7 @@ void ConnStateData::noteBodyConsumerAborted(BodyPipe::Pointer) STUB bool ConnStateData::handleReadData(char *buf, size_t size) STUB_RETVAL(false) bool ConnStateData::handleRequestBodyData() STUB_RETVAL(false) void ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServerConn, HttpRequest *request, CachePeer *peer, bool auth) STUB -void ConnStateData::unpinConnection() STUB +void ConnStateData::unpinConnection(bool andClose) STUB const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *request, const CachePeer *peer) STUB_RETVAL(NULL) void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) STUB void ConnStateData::clientReadRequest(const CommIoCbParams &io) STUB