From ce9bb79cc34c02977424d07579bb797494b0eb93 Mon Sep 17 00:00:00 2001
From: Christos Tsantilas <christos@chtsanti.net>
Date: Sun, 6 May 2018 11:00:38 +0000
Subject: [PATCH] Bug 4845: NegotiateSsl crash on aborting transaction (#201)

Security::PeerConnector::NegotiateSsl() might be called after the
Security::PeerConnector object is gone. This race condition is present
on both regular SSL and SslBump code paths, but sightings are rare.

This bug shares the underlying cause (and the solution) with bug 3505.
TODO: Adjust Comm::SetSelect() API to prevent future bugs like this.

This is a Measurement Factory project.
---
 src/security/PeerConnector.cc | 23 ++++++++++++++++-------
 src/security/PeerConnector.h  |  4 +++-
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index d3d99d5e65..11b9c4e575 100644
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -375,9 +375,18 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons
 void
 Security::PeerConnector::NegotiateSsl(int, void *data)
 {
-    PeerConnector *pc = static_cast<Security::PeerConnector *>(data);
+    const auto pc = static_cast<PeerConnector::Pointer*>(data);
+    if (pc->valid())
+        (*pc)->negotiateSsl();
+    delete pc;
+}
+
+/// Comm::SetSelect() callback. Direct calls tickle/resume negotiations.
+void
+Security::PeerConnector::negotiateSsl()
+{
     // Use job calls to add done() checks and other job logic/protections.
-    CallJobHere(83, 7, pc, Security::PeerConnector, negotiate);
+    CallJobHere(83, 7, this, Security::PeerConnector, negotiate);
 }
 
 void
@@ -460,19 +469,19 @@ Security::PeerConnector::noteWantRead()
 
             srvBio->holdRead(false);
             // schedule a negotiateSSl to allow openSSL parse received data
-            Security::PeerConnector::NegotiateSsl(fd, this);
+            negotiateSsl();
             return;
         } else if (srvBio->gotHelloFailed()) {
             srvBio->holdRead(false);
             debugs(83, DBG_IMPORTANT, "Error parsing SSL Server Hello Message on FD " << fd);
             // schedule a negotiateSSl to allow openSSL parse received data
-            Security::PeerConnector::NegotiateSsl(fd, this);
+            negotiateSsl();
             return;
         }
     }
 #endif
     setReadTimeout();
-    Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, this, 0);
+    Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, new Pointer(this), 0);
 }
 
 void
@@ -480,7 +489,7 @@ Security::PeerConnector::noteWantWrite()
 {
     const int fd = serverConnection()->fd;
     debugs(83, 5, serverConnection());
-    Comm::SetSelect(fd, COMM_SELECT_WRITE, &NegotiateSsl, this, 0);
+    Comm::SetSelect(fd, COMM_SELECT_WRITE, &NegotiateSsl, new Pointer(this), 0);
     return;
 }
 
@@ -673,7 +682,7 @@ Security::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus)
     }
 
     srvBio->holdRead(false);
-    Security::PeerConnector::NegotiateSsl(serverConnection()->fd, this);
+    negotiateSsl();
 }
 
 bool
diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h
index 682e5ff5cf..993031cd7d 100644
--- a/src/security/PeerConnector.h
+++ b/src/security/PeerConnector.h
@@ -65,6 +65,8 @@ class PeerConnector: virtual public AsyncJob
     CBDATA_CLASS(PeerConnector);
 
 public:
+    typedef CbcPointer<PeerConnector> Pointer;
+
     /// Callback dialer API to allow PeerConnector to set the answer.
     class CbDialer
     {
@@ -191,8 +193,8 @@ private:
     Security::CertErrors *sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &, Ssl::ErrorDetail *&);
 #endif
 
-    /// A wrapper function for negotiateSsl for use with Comm::SetSelect
     static void NegotiateSsl(int fd, void *data);
+    void negotiateSsl();
 
     /// The maximum allowed missing certificates downloads.
     static const unsigned int MaxCertsDownloads = 10;
-- 
2.39.5