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.

diff --git a/src/client_side.cc b/src/client_side.cc
index 68f9ebdc13824ffc9fccf7b53d7ecc2ab1b3edce..8ba88387c5ed07360077d499e7cb4ebd971b6208 100644 (file)
--- 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 6194b6cd9dd9b11e941cdd9a844b1d001d33815a..61410fb1ea47aa2b3b321a7627703584a0025083 100644 (file)
--- 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 a69817043cdf3d37e904545f73007e33528d4e76..a9fb15040a18df155bb33846b1030143691d7a13 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 6405b04f1ee0abac4f50516a286901fb9ccb3124..77768d3892ed7750147fdba483bafa88163b2a6a 100644 (file)
--- 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);