From b54a7c5a37afd21e43519644de32525656962125 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 2 Jun 2015 13:15:06 +0300 Subject: [PATCH] Bug3329: 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. --- src/client_side.cc | 5 +++-- src/comm/Connection.cc | 8 ++++++++ src/comm/Connection.h | 3 +++ src/ssl/PeerConnector.cc | 4 ++-- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/client_side.cc b/src/client_side.cc index 68f9ebdc13..8ba88387c5 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3611,7 +3611,7 @@ clientNegotiateSSL(int fd, void *data) int ret; if ((ret = Squid_SSL_accept(conn, clientNegotiateSSL)) <= 0) { if (ret < 0) // An error - comm_close(fd); + conn->clientConnection->close(); return; } @@ -4215,7 +4215,7 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data) connState->sslBumpMode = bumpAction; if (bumpAction == Ssl::bumpTerminate) { - comm_close(connState->clientConnection->fd); + connState->clientConnection->close(); } else if (bumpAction != Ssl::bumpSplice) { connState->startPeekAndSpliceDone(); } else @@ -4772,6 +4772,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 + pinning.serverConnection->noteClosure(); unpinConnection(false); if (sawZeroReply && clientConnection != NULL) { diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc index 6194b6cd9d..61410fb1ea 100644 --- a/src/comm/Connection.cc +++ b/src/comm/Connection.cc @@ -75,6 +75,14 @@ Comm::Connection::close() { if (isOpen()) { comm_close(fd); + noteClosure(); + } +} + +void +Comm::Connection::noteClosure() +{ + if (isOpen()) { fd = -1; if (CachePeer *p=getPeer()) peerConnClosed(p); diff --git a/src/comm/Connection.h b/src/comm/Connection.h index a69817043c..a9fb15040a 100644 --- a/src/comm/Connection.h +++ b/src/comm/Connection.h @@ -75,6 +75,9 @@ public: /** 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); } diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 6405b04f1e..77768d3892 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -259,8 +259,8 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action) } 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); -- 2.47.2