Avoid memory leaks when a certificate validator is used with SslBump

When a certificate validator was used, sslCrtvdHandleReplyWrapper delivered
validator response directly to the Ssl::PeerConnector job using job's
Ssl::CertValidationHelper::CVHCB callback. If that synchronous call happened
to be the last job call, then Ssl::PeerConnector::done() would become true
for the job, as it should, but nobody would notice that the PeerConnector
job object should be deleted, and the object would leak.

This fix converts CVHCB into an async job call to avoid direct, unprotected
job calls in this context.

This is a Measurement Factory project.

diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc
index 17ae06a7ea7ddfc4b1bd50ca9d6321d8e5b4fad8..40abcb23573d6581ef1e4cb9c6ae05fbaf0fee26 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -186,7 +186,8 @@ Ssl::PeerConnector::sslFinalized()
             validationRequest.errors = NULL;
         try {
             debugs(83, 5, "Sending SSL certificate for validation to ssl_crtvd.");
-            Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, sslCrtvdHandleReplyWrapper, this);
+            AsyncCall::Pointer call = asyncCall(83,5, "Ssl::PeerConnector::sslCrtvdHandleReply", Ssl::CertValidationHelper::CbDialer(this, &Ssl::PeerConnector::sslCrtvdHandleReply, nullptr));
+            Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, call);
             return false;
         } catch (const std::exception &e) {
             debugs(83, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtvd " <<
@@ -309,15 +310,10 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceGuess() const
 }
 
 void
-Ssl::PeerConnector::sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const &validationResponse)
+Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointer validationResponse)
 {
-    Ssl::PeerConnector *connector = (Ssl::PeerConnector *)(data);
-    connector->sslCrtvdHandleReply(validationResponse);
-}
+    Must(validationResponse != NULL);
 
-void
-Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse const &validationResponse)
-{
     Ssl::CertErrors *errs = NULL;
     Ssl::ErrorDetail *errDetails = NULL;
     bool validatorFailed = false;
@@ -325,11 +321,11 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse const &valid
         return;
     }
 
-    debugs(83,5, request->url.host() << " cert validation result: " << validationResponse.resultCode);
+    debugs(83,5, request->url.host() << " cert validation result: " << validationResponse->resultCode);
 
-    if (validationResponse.resultCode == ::Helper::Error)
-        errs = sslCrtvdCheckForErrors(validationResponse, errDetails);
-    else if (validationResponse.resultCode != ::Helper::Okay)
+    if (validationResponse->resultCode == ::Helper::Error)
+        errs = sslCrtvdCheckForErrors(*validationResponse, errDetails);
+    else if (validationResponse->resultCode != ::Helper::Okay)
         validatorFailed = true;
 
     if (!errDetails && !validatorFailed) {

diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h
index 769df69d5f6dc636995a7471521c7b1fd7125dae..5d5548b3509b8bdd87afa84c8bc3ac3c4624ccac 100644 (file)
--- a/src/ssl/PeerConnector.h
+++ b/src/ssl/PeerConnector.h
@@ -12,6 +12,7 @@
 #include "acl/Acl.h"
 #include "base/AsyncCbdataCalls.h"
 #include "base/AsyncJob.h"
+#include "CommCalls.h"
 #include "security/EncryptorAnswer.h"
 #include "ssl/support.h"
 #include <iosfwd>
@@ -24,6 +25,7 @@ namespace Ssl
 
 class ErrorDetail;
 class CertValidationResponse;
+typedef RefCount<CertValidationResponse> CertValidationResponsePointer;
 
 /**
  \par
@@ -160,14 +162,11 @@ private:
     void callBack();
 
     /// Process response from cert validator helper
-    void sslCrtvdHandleReply(Ssl::CertValidationResponse const &);
+    void sslCrtvdHandleReply(Ssl::CertValidationResponsePointer);
 
     /// Check SSL errors returned from cert validator against sslproxy_cert_error access list
     Ssl::CertErrors *sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &, Ssl::ErrorDetail *&);
 
-    /// Callback function called when squid receive message from cert validator helper
-    static void sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const &);
-
     /// A wrapper function for negotiateSsl for use with Comm::SetSelect
     static void NegotiateSsl(int fd, void *data);
     AsyncCall::Pointer callback; ///< we call this with the results

diff --git a/src/ssl/cert_validate_message.h b/src/ssl/cert_validate_message.h
index 3fcd6b7df245a6c9cc0b8533fa6a84e1dea8ff3b..2aa4e84d8361c858c77d36fe8e95bdca01501ece 100644 (file)
--- a/src/ssl/cert_validate_message.h
+++ b/src/ssl/cert_validate_message.h
@@ -9,6 +9,7 @@
 #ifndef SQUID_SSL_CERT_VALIDATE_MESSAGE_H
 #define SQUID_SSL_CERT_VALIDATE_MESSAGE_H
 
+#include "base/RefCount.h"
 #include "helper/ResultCode.h"
 #include "ssl/crtd_message.h"
 #include "ssl/support.h"
@@ -35,9 +36,11 @@ public:
  * This class is used to store informations found in certificate validation
  * response messages read from certificate validator helper
  */
-class CertValidationResponse
+class CertValidationResponse: public RefCountable
 {
 public:
+    typedef RefCount<CertValidationResponse> Pointer;
+
     /**
      * This class used to hold error informations returned from
      * cert validator helper.

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index e141018a7ec1243d1fb5f8440efd1829c0acf474..72e2debb76232132bf08ea7c03b75a1cc4145c5e 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -19,7 +19,7 @@
 #include "ssl/helper.h"
 #include "wordlist.h"
 
-LruMap<Ssl::CertValidationResponse> *Ssl::CertValidationHelper::HelperCache = NULL;
+Ssl::CertValidationHelper::LruCache *Ssl::CertValidationHelper::HelperCache = nullptr;
 
 #if USE_SSL_CRTD
 Ssl::Helper * Ssl::Helper::GetInstance()
@@ -173,7 +173,7 @@ void Ssl::CertValidationHelper::Init()
 
     //WARNING: initializing static member in an object initialization method
     assert(HelperCache == NULL);
-    HelperCache = new LruMap<Ssl::CertValidationResponse>(ttl, cache);
+    HelperCache = new Ssl::CertValidationHelper::LruCache(ttl, cache);
 }
 
 void Ssl::CertValidationHelper::Shutdown()
@@ -198,8 +198,7 @@ class submitData
 
 public:
     std::string query;
-    Ssl::CertValidationHelper::CVHCB *callback;
-    void *data;
+    AsyncCall::Pointer callback;
     SSL *ssl;
 };
 CBDATA_CLASS_INIT(submitData);
@@ -208,7 +207,7 @@ static void
 sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply)
 {
     Ssl::CertValidationMsg replyMsg(Ssl::CrtdMessage::REPLY);
-    Ssl::CertValidationResponse *validationResponse = new Ssl::CertValidationResponse;
+    Ssl::CertValidationResponse::Pointer validationResponse = new Ssl::CertValidationResponse;
     std::string error;
 
     submitData *crtdvdData = static_cast<submitData *>(data);
@@ -224,20 +223,22 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply)
     } else
         validationResponse->resultCode = reply.result;
 
-    crtdvdData->callback(crtdvdData->data, *validationResponse);
+    Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast<Ssl::CertValidationHelper::CbDialer*>(crtdvdData->callback->getDialer());
+    Must(dialer);
+    dialer->arg1 = validationResponse;
+    ScheduleCallHere(crtdvdData->callback);
 
     if (Ssl::CertValidationHelper::HelperCache &&
             (validationResponse->resultCode == ::Helper::Okay || validationResponse->resultCode == ::Helper::Error)) {
-        Ssl::CertValidationHelper::HelperCache->add(crtdvdData->query.c_str(), validationResponse);
-    } else
-        delete validationResponse;
+        Ssl::CertValidationResponse::Pointer *item = new Ssl::CertValidationResponse::Pointer(validationResponse);
+        Ssl::CertValidationHelper::HelperCache->add(crtdvdData->query.c_str(), item);
+    }
 
-    cbdataReferenceDone(crtdvdData->data);
     SSL_free(crtdvdData->ssl);
     delete crtdvdData;
 }
 
-void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &request, Ssl::CertValidationHelper::CVHCB * callback, void * data)
+void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &request, AsyncCall::Pointer &callback)
 {
     assert(ssl_crt_validator);
 
@@ -250,26 +251,28 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ
     crtdvdData->query = message.compose();
     crtdvdData->query += '\n';
     crtdvdData->callback = callback;
-    crtdvdData->data = cbdataReference(data);
     crtdvdData->ssl = request.ssl;
     CRYPTO_add(&crtdvdData->ssl->references,1,CRYPTO_LOCK_SSL);
-    Ssl::CertValidationResponse const*validationResponse;
+    Ssl::CertValidationResponse::Pointer const*validationResponse;
 
     if (CertValidationHelper::HelperCache &&
             (validationResponse = CertValidationHelper::HelperCache->get(crtdvdData->query.c_str()))) {
-        callback(data, *validationResponse);
-        cbdataReferenceDone(crtdvdData->data);
+
+        CertValidationHelper::CbDialer *dialer = dynamic_cast<CertValidationHelper::CbDialer*>(callback->getDialer());
+        dialer->arg1 = *validationResponse;
+        ScheduleCallHere(callback);
         SSL_free(crtdvdData->ssl);
         delete crtdvdData;
         return;
     }
 
     if (!ssl_crt_validator->trySubmit(crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData)) {
-        Ssl::CertValidationResponse resp;
-        resp.resultCode = ::Helper::BrokenHelper;
-        callback(data, resp);
+        Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse;;
+        resp->resultCode = ::Helper::BrokenHelper;
+        Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast<Ssl::CertValidationHelper::CbDialer*>(callback->getDialer());
+        dialer->arg1 = resp;
+        ScheduleCallHere(callback);
 
-        cbdataReferenceDone(crtdvdData->data);
         SSL_free(crtdvdData->ssl);
         delete crtdvdData;
         return;

diff --git a/src/ssl/helper.h b/src/ssl/helper.h
index 69cb15c1998f205bef812ba20c1c0199d9e3f2d0..140f2e78b98cd445fb18dbca7f8a0610c2ec81a5 100644 (file)
--- a/src/ssl/helper.h
+++ b/src/ssl/helper.h
@@ -9,6 +9,7 @@
 #ifndef SQUID_SSL_HELPER_H
 #define SQUID_SSL_HELPER_H
 
+#include "base/AsyncJobCalls.h"
 #include "base/LruMap.h"
 #include "helper/forward.h"
 #include "ssl/cert_validate_message.h"
@@ -38,24 +39,28 @@ private:
 };
 #endif
 
+class PeerConnector;
 class CertValidationRequest;
 class CertValidationResponse;
 class CertValidationHelper
 {
 public:
+    typedef UnaryMemFunT<Ssl::PeerConnector, CertValidationResponse::Pointer> CbDialer;
+
     typedef void CVHCB(void *, Ssl::CertValidationResponse const &);
     static CertValidationHelper * GetInstance(); ///< Instance class.
     void Init(); ///< Init helper structure.
     void Shutdown(); ///< Shutdown helper structure.
     /// Submit crtd request message to external crtd server.
-    void sslSubmit(Ssl::CertValidationRequest const & request, CVHCB * callback, void *data);
+    void sslSubmit(Ssl::CertValidationRequest const & request, AsyncCall::Pointer &);
 private:
     CertValidationHelper();
     ~CertValidationHelper();
 
     helper * ssl_crt_validator; ///< helper for management of ssl_crtd.
 public:
-    static LruMap<Ssl::CertValidationResponse> *HelperCache; ///< cache for cert validation helper
+    typedef LruMap<Ssl::CertValidationResponse::Pointer, sizeof(Ssl::CertValidationResponse::Pointer) + sizeof(Ssl::CertValidationResponse)> LruCache;
+    static LruCache *HelperCache; ///< cache for cert validation helper
 };
 
 } //namespace Ssl