Improved Security::PeerConnector::serverConn management

Similar to the previous branch commit

When sending a negative answer, we would set answer().conn to an open
connection, async-send the answer, and then hurry to close the
connection using our pointer to the shared Connection object. If
everything went according to the plan, the recipient would get a non-nil
but closed Connection object. Now, a negative answer simply means no
connection at all. Same for a tunneled answer.

Probably fixed a bug in the official code where
PeerConnector::negotiate() assumed that a sslFinalized() does not return
true after callBack(). It may (e.g., when CertValidationHelper::Submit()
throws). Same for PeekingPeerConnector::checkForPeekAndSpliceMatched().

Also reduced Security::PeerConnector exposure to a closed Connection
object when serverConn is closed externally.

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 4371b8ea5d8cad6dbf66db94e2c86170f930391e..c02b5833bfa30125b28b9415e6cfc186ea0951eb 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -1001,9 +1001,10 @@ FwdState::connectedToPeer(Security::EncryptorAnswer &answer)
 
     ErrorState *error = nullptr;
     if ((error = answer.error.get())) {
-        Must(!Comm::IsConnOpen(answer.conn));
+        assert(!answer.conn);
         answer.error.clear(); // preserve error for errorSendComplete()
     } else if (answer.tunneled) {
+        assert(!answer.conn);
         // TODO: When ConnStateData establishes tunnels, its state changes
         // [in ways that may affect logging?]. Consider informing
         // ConnStateData about our tunnel or otherwise unifying tunnel

diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc
index 996997d57ddacc644eb09bed790af54205ca073d..9e0bf1b4d0347c1c6e921a9feba06469c9e8fd4a 100644 (file)
--- a/src/PeerPoolMgr.cc
+++ b/src/PeerPoolMgr.cc
@@ -145,14 +145,16 @@ PeerPoolMgr::handleSecuredPeer(Security::EncryptorAnswer &answer)
         return;
     }
 
+    assert(!answer.tunneled);
     if (answer.error.get()) {
-        if (answer.conn != NULL)
-            answer.conn->close();
+        assert(!answer.conn);
         // PeerConnector calls peerConnectFailed() for us;
         checkpoint("conn securing failure"); // may retry
         return;
     }
 
+    // TODO: Check for .closing() like FwdState::connectedToPeer() does?
+
     pushNewConnection(answer.conn);
 }
 

diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc
index 77360fb88d466e4b453ee00e1ecd750a5a6499ed..6e4cf75ab4d183644084f31023f8ff05570b3266 100644 (file)
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -714,10 +714,9 @@ Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer)
 {
     encryptionWait.finish();
 
+    assert(!answer.tunneled);
     if (answer.error.get()) {
-        // XXX: Security::PeerConnector should do that for negative answers instead.
-        if (answer.conn != NULL)
-            answer.conn->close();
+        assert(!answer.conn);
         // TODO: Refactor dieOnConnectionFailure() to be usable here as well.
         debugs(93, 2, typeName <<
                " TLS negotiation to " << service().cfg().uri << " failed");
@@ -729,6 +728,7 @@ Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer)
     debugs(93, 5, "TLS negotiation to " << service().cfg().uri << " complete");
 
     // XXX: answer.conn could be closing here. Missing a syncWithComm equivalent?
+    // TODO: Check for .closing() like FwdState::connectedToPeer() does?
     useIcapConnection(answer.conn);
 }
 

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index d28169f4df6fa3392104b489c743b8d86cfb97a2..d06e281ee2ba8344be8e3fc48916504978286ca2 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -92,9 +92,7 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams &params)
 
     closeHandler = nullptr;
     if (serverConn) {
-        // TODO: Calling PconnPool::noteUses() should not be our responsibility.
-        if (noteFwdPconnUse && serverConn->isOpen())
-            fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
+        countFailingConnection();
         serverConn->noteClosure();
         serverConn = nullptr;
     }
@@ -223,7 +221,8 @@ Security::PeerConnector::negotiate()
     if (!sslFinalized())
         return;
 
-    sendSuccess();
+    if (callback) // true sslFinalized() may bail(), calling the callback
+        sendSuccess();
 }
 
 bool
@@ -298,7 +297,8 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe
 
     if (!errDetails && !validatorFailed) {
         noteNegotiationDone(NULL);
-        sendSuccess();
+        if (callback)
+            sendSuccess();
         return;
     }
 
@@ -563,66 +563,76 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error
     bail(anErr);
 }
 
+Security::EncryptorAnswer &
+Security::PeerConnector::answer()
+{
+    assert(callback);
+    const auto dialer = dynamic_cast<CbDialer*>(callback->getDialer());
+    assert(dialer);
+    return dialer->answer();
+}
+
 void
 Security::PeerConnector::bail(ErrorState *error)
 {
     Must(error); // or the recipient will not know there was a problem
-    Must(callback != NULL);
-    CbDialer *dialer = dynamic_cast<CbDialer*>(callback->getDialer());
-    Must(dialer);
-    dialer->answer().error = error;
-
-    if (serverConnection()) {
-        if (const auto p = serverConnection()->getPeer())
-            peerConnectFailed(p);
+    answer().error = error;
+
+    if (const auto failingConnection = serverConn) {
+        countFailingConnection();
+        disconnect();
+        failingConnection->close();
     }
 
     callBack();
-    disconnect();
-
-    // TODO: Close before callBack(); do not pretend to send an open connection.
-    if (Comm::IsConnOpen(serverConn)) {
-        if (noteFwdPconnUse)
-            fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
-        assert(!closeHandler); // the above disconnect() removes it
-        serverConn->close();
-    }
-    serverConn = nullptr;
 }
 
 void
 Security::PeerConnector::sendSuccess()
 {
-    callBack();
+    assert(Comm::IsConnOpen(serverConn));
+    answer().conn = serverConn;
     disconnect();
+    callBack();
+}
+
+void
+Security::PeerConnector::countFailingConnection()
+{
+    assert(serverConn);
+    if (const auto p = serverConn->getPeer())
+        peerConnectFailed(p);
+    // TODO: Calling PconnPool::noteUses() should not be our responsibility.
+    if (noteFwdPconnUse && serverConn->isOpen())
+        fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses);
 }
 
 void
 Security::PeerConnector::disconnect()
 {
-    if (!Comm::IsConnOpen(serverConnection()))
-        return;
+    const auto stillOpen = Comm::IsConnOpen(serverConn);
 
     if (closeHandler) {
-        comm_remove_close_handler(serverConnection()->fd, closeHandler);
+        if (stillOpen)
+            comm_remove_close_handler(serverConn->fd, closeHandler);
         closeHandler = nullptr;
     }
 
-    commUnsetConnTimeout(serverConnection());
+    if (stillOpen)
+        commUnsetConnTimeout(serverConn);
+
+    serverConn = nullptr;
 }
 
 void
 Security::PeerConnector::callBack()
 {
-    debugs(83, 5, "TLS setup ended for " << serverConnection());
+    debugs(83, 5, "TLS setup ended for " << answer().conn);
 
     AsyncCall::Pointer cb = callback;
     // Do this now so that if we throw below, swanSong() assert that we _tried_
     // to call back holds.
     callback = NULL; // this should make done() true
-    CbDialer *dialer = dynamic_cast<CbDialer*>(cb->getDialer());
-    Must(dialer);
-    dialer->answer().conn = serverConnection(); // may be nil
     ScheduleCallHere(cb);
 }
 

diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h
index 46bcca963f6fec74de2681c3fde62c3014bd957f..f886b6ee761cde3bc9cd54c644b89cecbbe3522e 100644 (file)
--- a/src/security/PeerConnector.h
+++ b/src/security/PeerConnector.h
@@ -150,6 +150,9 @@ protected:
     /// a bail(), sendSuccess() helper: stops monitoring the connection
     void disconnect();
 
+    /// updates connection usage history before the connection is closed
+    void countFailingConnection();
+
     /// If called the certificates validator will not used
     void bypassCertValidator() {useCertValidator_ = false;}
 
@@ -157,6 +160,9 @@ protected:
     /// logging
     void recordNegotiationDetails();
 
+    /// convenience method to get to the answer fields
+    EncryptorAnswer &answer();
+
     HttpRequestPointer request; ///< peer connection trigger or cause
     Comm::ConnectionPointer serverConn; ///< TCP connection to the peer
     AccessLogEntryPointer al; ///< info for the future access.log entry

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index e48e39b35b174f62c82286eef9a07f92b1c1c6a1..088f8290d6da8063c09ef719721ae98f94a8c428 100644 (file)
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -105,10 +105,8 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched(const Ssl::BumpMode acti
         splice = true;
         // Ssl Negotiation stops here. Last SSL checks for valid certificates
         // and if done, switch to tunnel mode
-        if (sslFinalized()) {
-            debugs(83,5, "Abort NegotiateSSL on FD " << serverConn->fd << " and splice the connection");
+        if (sslFinalized() && callback)
             callBack();
-        }
     }
 }
 
@@ -270,8 +268,11 @@ Ssl::PeekingPeerConnector::startTunneling()
     auto b = SSL_get_rbio(session.get());
     auto srvBio = static_cast<Ssl::ServerBio*>(BIO_get_data(b));
 
+    debugs(83, 5, "will tunnel instead of negotiating TLS");
     switchToTunnel(request.getRaw(), clientConn, serverConn, srvBio->rBufData());
-    tunnelInsteadOfNegotiating();
+    answer().tunneled = true;
+    disconnect();
+    callBack();
 }
 
 void
@@ -392,13 +393,4 @@ Ssl::PeekingPeerConnector::serverCertificateVerified()
     }
 }
 
-void
-Ssl::PeekingPeerConnector::tunnelInsteadOfNegotiating()
-{
-    Must(callback != NULL);
-    CbDialer *dialer = dynamic_cast<CbDialer*>(callback->getDialer());
-    Must(dialer);
-    dialer->answer().tunneled = true;
-    debugs(83, 5, "The SSL negotiation with server aborted");
-}
 

diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc
index 9ee690ec4bb976e0eaa906a8466d9ef03c021228..e11d62a5642a5c3ce82b02ce268083cdfbeeb714 100644 (file)
--- a/src/tests/stub_libsecurity.cc
+++ b/src/tests/stub_libsecurity.cc
@@ -70,7 +70,9 @@ void PeerConnector::bail(ErrorState *) STUB
 void PeerConnector::sendSuccess() STUB
 void PeerConnector::callBack() STUB
 void PeerConnector::disconnect() STUB
+void PeerConnector::countFailingConnection() STUB
 void PeerConnector::recordNegotiationDetails() STUB
+EncryptorAnswer &PeerConnector::answer() STUB_RETREF(EncryptorAnswer)
 }
 
 #include "security/PeerOptions.h"

diff --git a/src/tunnel.cc b/src/tunnel.cc
index 6d5eddd447acf25f42cf71003a15380c0f8a2713..b7ce574cb3c82ed68b935f68cca0d47d658e9f48 100644 (file)
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -979,7 +979,7 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer)
 
     if (!answer.positive()) {
         sawProblem = true;
-        Must(!answer.conn);
+        assert(!answer.conn);
     } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) {
         sawProblem = true;
         closePendingConnection(answer.conn, "conn was closed while waiting for tunnelEstablishmentDone");
@@ -1199,8 +1199,9 @@ TunnelStateData::noteSecurityPeerConnectorAnswer(Security::EncryptorAnswer &answ
     encryptionWait.finish();
 
     ErrorState *error = nullptr;
+    assert(!answer.tunneled);
     if ((error = answer.error.get())) {
-        Must(!Comm::IsConnOpen(answer.conn));
+        assert(!answer.conn);
         answer.error.clear();
     } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) {
         error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request.getRaw(), al);