Bug 4405: assertion failed: comm.cc:554: "Comm::IsConnOpen(conn)"

 It is possible that the connection will be closed somewhere inside
"clientTunnelOnError" call, inside ConnStateData::fakeAConnectRequest which
is called by ConnStateData::clientTunnelOnError or inside spliceOnError()
while trying to splice(). In this case the callers should be informed to abort
imediatelly, but instead continues, and try to set timeout handler on closed
connection.

This patch:
  - Modify ConnStateData::fakeAConnectRequest and ConnStateData::splice methods     to return boolean and false on error.
  - Does not close the connection inside ConnStateData::fakeAConnectRequest but
    instead return false and allow callers to close the connection if required.

This is a Measurement Factory project

diff --git a/src/client_side.cc b/src/client_side.cc
index 03dcfd53a420e684a2be5627579de2981bca5d4e..465ef9d52f3b30977dd3059623f3c465ca34d562 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1578,8 +1578,7 @@ clientTunnelOnError(ConnStateData *conn, Http::Stream *context, HttpRequest *req
                 conn->pipeline.popMe(Http::StreamPointer(context));
             }
             Comm::SetSelect(conn->clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0);
-            conn->fakeAConnectRequest("unknown-protocol", conn->preservedClientData);
-            return true;
+            return conn->fakeAConnectRequest("unknown-protocol", conn->preservedClientData);
         } else {
             debugs(33, 3, "Continue with returning the error: " << requestError);
         }
@@ -2763,7 +2762,8 @@ httpsSslBumpAccessCheckDone(allow_t answer, void *data)
         debugs(33, 2, HERE << "sslBump not needed for " << connState->clientConnection);
         connState->sslBumpMode = Ssl::bumpNone;
     }
-    connState->fakeAConnectRequest("ssl-bump", connState->inBuf);
+    if (!connState->fakeAConnectRequest("ssl-bump", connState->inBuf))
+        connState->clientConnection->close();
 }
 
 /** handle a new HTTPS connection */
@@ -3142,8 +3142,7 @@ ConnStateData::spliceOnError(const err_type err)
         checklist.conn(this);
         allow_t answer = checklist.fastCheck();
         if (answer == ACCESS_ALLOWED && answer.kind == 1) {
-            splice();
-            return true;
+            return splice();
         }
     }
     return false;
@@ -3245,11 +3244,11 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data)
         connState->clientConnection->close();
     } else if (bumpAction != Ssl::bumpSplice) {
         connState->startPeekAndSpliceDone();
-    } else
-        connState->splice();
+    } else if (!connState->splice())
+        connState->clientConnection->close();
 }
 
-void
+bool
 ConnStateData::splice()
 {
     // normally we can splice here, because we just got client hello message
@@ -3273,7 +3272,7 @@ ConnStateData::splice()
         // XXX: copy from MemBuf reallocates, not a regression since old code did too
         SBuf temp;
         temp.append(rbuf.content(), rbuf.contentSize());
-        fakeAConnectRequest("intercepted TLS spliced", temp);
+        return fakeAConnectRequest("intercepted TLS spliced", temp);
     } else {
         // XXX: assuming that there was an HTTP/1.1 CONNECT to begin with...
 
@@ -3284,6 +3283,7 @@ ConnStateData::splice()
         Http::StreamPointer context = pipeline.front();
         ClientHttpRequest *http = context->http;
         tunnelStart(http);
+        return true;
     }
 }
 
@@ -3350,7 +3350,7 @@ ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection)
 
 #endif /* USE_OPENSSL */
 
-void
+bool
 ConnStateData::fakeAConnectRequest(const char *reason, const SBuf &payload)
 {
     // fake a CONNECT request to force connState to tunnel
@@ -3381,8 +3381,9 @@ ConnStateData::fakeAConnectRequest(const char *reason, const SBuf &payload)
 
     if (!ret) {
         debugs(33, 2, "Failed to start fake CONNECT request for " << reason << " connection: " << clientConnection);
-        clientConnection->close();
+        return false;
     }
+    return true;
 }
 
 /// check FD after clientHttp[s]ConnectionOpened, adjust HttpSockets as needed

diff --git a/src/client_side.h b/src/client_side.h
index fc7a7a0898dd9cb5371186e86379e99127a6b89e..009c7097ef8aa8571ab2c73d29abf06d8dd57313 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -213,7 +213,7 @@ public:
     void httpsPeeked(Comm::ConnectionPointer serverConnection);
 
     /// Splice a bumped client connection on peek-and-splice mode
-    void splice();
+    bool splice();
 
     /// Check on_unsupported_protocol access list and splice if required
     /// \retval true on splice
@@ -280,7 +280,7 @@ public:
 
     /// generate a fake CONNECT request with the given payload
     /// at the beginning of the client I/O buffer
-    void fakeAConnectRequest(const char *reason, const SBuf &payload);
+    bool fakeAConnectRequest(const char *reason, const SBuf &payload);
 
     /// client data which may need to forward as-is to server after an
     /// on_unsupported_protocol tunnel decision.