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 317110545908c8e7f1604c2ad48907f38fce6def..9f14bd375cc80e255f95c52235750f7a4c9aa8fe 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;
 }
 
@@ -677,7 +686,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;