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.

diff --git a/src/client_side.cc b/src/client_side.cc
index a75a5534f24dd27079c1846200c84221daaef2f8..c06e085934e2dafc9447066c632d1e927f49473e 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3687,19 +3687,19 @@ Squid_SSL_accept(ConnStateData *conn, PF *callback)
                 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 @@ 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 {
@@ -4851,6 +4851,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 6bf8ad50cd835afce0b89bbeaaf0a43813d65475..ef2b3c59f46d846415f729ca57052c573fb9d406 100644 (file)
--- a/src/comm/Connection.cc
+++ b/src/comm/Connection.cc
@@ -74,6 +74,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 46aabece84808dace3813bbbc09bb81ccf2e9eab..09771c232057a54053aecff7cf4c629189305d19 100644 (file)
--- 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 98c779d021c24994ec9ecba5f4f864dd9b3f75b9..b4dfd8ff1a4a9d1514896bfa02ddb2c537b0d13a 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -393,8 +393,8 @@ Ssl::PeerConnector::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);