------------------------------------------------------------ revno: 13842 revision-id: squid3@treenet.co.nz-20150605232222-i3fy2s83ojbobudm parent: squid3@treenet.co.nz-20150528110147-8qw03zwink9oh3be fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=3329 author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Fri 2015-06-05 16:22:22 -0700 message: Bug 3329: The server side pinned connection is not closed properly ... in ConnStateData::clientPinnedConnectionClosed CommClose handler. Squid enters a buggy state when an idle connection pinned to a peer closes: - The ConnStateData::clientPinnedConnectionRead, the pinned peer connection read handler, is called with the io.flag set to Comm::ERR_CLOSING. The read handler does not close the peer Comm::Connection object. This is correct and expected -- the I/O handler must exit on ERR_CLOSING without doing anything. - The ConnStateData::clientPinnedConnectionClosed close handler is called, but it does not close the peer Comm::Connection object either. Again, this is correct and expected -- the close handler is not the place to close a being-closed connection. - The corresponding fde object is marked as closed (fde::flags.open is false), but the peer Comm::Connection object is still open (Comm::Connection.fd >= 0)! From this point on, we have an inconsistency between the peer Comm::Connection object state and the real world. - When the ConnStateData::pinning::serverConnection object is later destroyed (by refcounting), it will try to close its fd. If that fd is already in use (e.g., by another Comm::Connection), bad things happen (crashes, segfaults, etc). Otherwise (i.e., if that fd is not open), comm_close may cry about BUG 3556 (or worse). To fix this problem, we must not allow Comm::Connections to get out of sync with fd_table, even when a descriptor is closed without going through Connection::close(). There are two ways to accomplished that: * Change Comm to always store Comm::Connections and similar high-level objects instead of fdes. This is a huge change that has been long on the TODO list (those "other high-level objects" is on of the primary obstacles there because not everything with a FD is a Connection). * Notify Comm::Connections about closure in their closing handlers (this change). This design relies on every Comm::Connection having a close handler that notifies it. It may take us some time to reach that goal, but this change is the first step providing the necessary API, a known bug fix, and a few preventive changes. This change: - Adds a new Comm::Connection::noteClosure() method to inform the Comm::Connection object that somebody is closing its FD. - Uses the new method inside ConnStateData::clientPinnedConnectionClosed handler to inform the ConnStateData::pinning::serverConnection object that its FD is being closed. - Replaces comm_close calls which may cause bug #3329 in other places with Comm::Connection->close() calls. Initially based on Nathan Hoad research for bug 3329. This is a Measurement Factory project. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20150605232222-i3fy2s83ojbobudm # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 13157e42a62da91077b99e2d4369ba3b6b31a011 # timestamp: 2015-06-05 23:50:54 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20150528110147-\ # 8qw03zwink9oh3be # # Begin patch === modified file 'src/client_side.cc' --- src/client_side.cc 2015-05-22 05:01:24 +0000 +++ src/client_side.cc 2015-06-05 23:22:22 +0000 @@ -3687,19 +3687,19 @@ debugs(83, (xerrno == ECONNRESET) ? 1 : 2, "Error negotiating SSL connection on FD " << fd << ": " << (xerrno == 0 ? ERR_error_string(ssl_error, NULL) : xstrerr(xerrno))); } - comm_close(fd); + conn->clientConnection->close(); return false; case SSL_ERROR_ZERO_RETURN: debugs(83, DBG_IMPORTANT, "Error negotiating SSL connection on FD " << fd << ": Closed by client"); - comm_close(fd); + conn->clientConnection->close(); return false; default: debugs(83, DBG_IMPORTANT, "Error negotiating SSL connection on FD " << fd << ": " << ERR_error_string(ERR_get_error(), NULL) << " (" << ssl_error << "/" << ret << ")"); - comm_close(fd); + conn->clientConnection->close(); return false; } @@ -4306,7 +4306,7 @@ connState->sslBumpMode = bumpAction; if (bumpAction == Ssl::bumpTerminate) { - comm_close(connState->clientConnection->fd); + connState->clientConnection->close(); } else if (bumpAction != Ssl::bumpSplice) { connState->startPeekAndSpliceDone(); } else { @@ -4851,6 +4851,7 @@ assert(pinning.serverConnection == io.conn); pinning.closeHandler = NULL; // Comm unregisters handlers before calling const bool sawZeroReply = pinning.zeroReply; // reset when unpinning + pinning.serverConnection->noteClosure(); unpinConnection(false); if (sawZeroReply && clientConnection != NULL) { === modified file 'src/comm/Connection.cc' --- src/comm/Connection.cc 2015-01-13 09:13:49 +0000 +++ src/comm/Connection.cc 2015-06-05 23:22:22 +0000 @@ -74,6 +74,14 @@ { if (isOpen()) { comm_close(fd); + noteClosure(); + } +} + +void +Comm::Connection::noteClosure() +{ + if (isOpen()) { fd = -1; if (CachePeer *p=getPeer()) peerConnClosed(p); === modified file 'src/comm/Connection.h' --- src/comm/Connection.h 2015-01-13 09:13:49 +0000 +++ src/comm/Connection.h 2015-06-05 23:22:22 +0000 @@ -75,6 +75,9 @@ /** Close any open socket. */ void close(); + /** Synchronize with Comm: Somebody closed our connection. */ + void noteClosure(); + /** determine whether this object describes an active connection or not. */ bool isOpen() const { return (fd >= 0); } === modified file 'src/ssl/PeerConnector.cc' --- src/ssl/PeerConnector.cc 2015-04-26 16:44:23 +0000 +++ src/ssl/PeerConnector.cc 2015-06-05 23:22:22 +0000 @@ -393,8 +393,8 @@ } if (finalAction == Ssl::bumpTerminate) { - comm_close(serverConn->fd); - comm_close(clientConn->fd); + serverConn->close(); + clientConn->close(); } else if (finalAction != Ssl::bumpSplice) { //Allow write, proceed with the connection srvBio->holdWrite(false);