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.

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index d3d99d5e65e4af7a082f7d5422c61a1e1fe44e96..11b9c4e575150140f0bf034690d2045b77bcc430 100644 (file)
--- 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 682e5ff5cf0833d44b46cdcd0c83c8fbdb412e55..993031cd7db6bc864833addf5bc9480fdddff740 100644 (file)
--- 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;