From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 6 Aug 2021 19:06:13 +0000 (-0400)
Subject: Removed branch-added try/catch fixing orphaned params.conn
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=48be7128c0c9c1a97146dc4bbd296a8b56e03f66;p=thirdparty%2Fsquid.git

Removed branch-added try/catch fixing orphaned params.conn

The problem is real, but there are other similarly problematic places in
the official code. If we fix this one place, then we probably should fix
all other known ones. However,

* the related exceptions ought be rare (there are no known ones);
* Squid will probably recover even if the affected Connection objects
  are orphaned; and
* the appropriate fix (i.e. custom try/catch blocks) is not the right
  long-term solution -- we need a better Connection storage API that
  would do the right thing automatically.

Given the combination of these factors, I think it is best to do nothing
here/now but add the right long-term API in a dedicated branch.
---

diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc
index 678fda0040..5e8d9986d1 100644
--- a/src/PeerPoolMgr.cc
+++ b/src/PeerPoolMgr.cc
@@ -108,34 +108,21 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams &params)
 
     // Handle TLS peers.
     if (peer->secure.encryptTransport) {
-        encryptTransport(params.conn);
-        return;
-    }
-
-    pushNewConnection(params.conn);
-}
-
-void
-PeerPoolMgr::encryptTransport(const Comm::ConnectionPointer &conn)
-{
-    try {
+        // XXX: Exceptions orphan params.conn
         AsyncCall::Pointer callback = asyncCall(48, 4, "PeerPoolMgr::handleSecuredPeer",
                                                 MyAnswerDialer(this, &PeerPoolMgr::handleSecuredPeer));
 
         const auto peerTimeout = peer->connectTimeout();
-        const int timeUsed = squid_curtime - conn->startTime();
+        const int timeUsed = squid_curtime - params.conn->startTime();
         // Use positive timeout when less than one second is left for conn.
         const int timeLeft = positiveTimeout(peerTimeout - timeUsed);
-        const auto connector = new Security::BlindPeerConnector(request, conn, callback, nullptr, timeLeft);
+        const auto connector = new Security::BlindPeerConnector(request, params.conn, callback, nullptr, timeLeft);
         encryptionWait.start(connector, callback);
         AsyncJob::Start(connector); // will call our callback
+        return;
     }
-    catch (...) {
-        conn->close();
-        // We could report and continue if we can recover from this failure, but
-        // it is difficult to determine/do that correctly so lets KISS for now.
-        throw;
-    }
+
+    pushNewConnection(params.conn);
 }
 
 void
diff --git a/src/PeerPoolMgr.h b/src/PeerPoolMgr.h
index 54af79973c..ae07c1fd1c 100644
--- a/src/PeerPoolMgr.h
+++ b/src/PeerPoolMgr.h
@@ -52,9 +52,6 @@ protected:
     /// Comm::ConnOpener calls this when done opening a connection for us
     void handleOpenedConnection(const CommConnectCbParams &params);
 
-    /// initiates Security::PeerConnector work on a just-established connection
-    void encryptTransport(const Comm::ConnectionPointer &);
-
     /// Security::PeerConnector callback
     void handleSecuredPeer(Security::EncryptorAnswer &answer);