From: Christos Tsantilas Date: Sun, 13 Dec 2015 17:13:44 +0000 (+0200) Subject: Avoid memory leaks when a certificate validator is used with SslBump X-Git-Tag: SQUID_4_0_4~43 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0e208dadae1eab0f5c5b4facca9a84e4da5a2376;p=thirdparty%2Fsquid.git 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 17ae06a7ea..40abcb2357 100644 --- 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 769df69d5f..5d5548b350 100644 --- 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 @@ -24,6 +25,7 @@ namespace Ssl class ErrorDetail; class CertValidationResponse; +typedef RefCount 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 3fcd6b7df2..2aa4e84d83 100644 --- 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 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 e141018a7e..72e2debb76 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -19,7 +19,7 @@ #include "ssl/helper.h" #include "wordlist.h" -LruMap *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(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(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(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(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(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 69cb15c199..140f2e78b9 100644 --- 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 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 *HelperCache; ///< cache for cert validation helper + typedef LruMap LruCache; + static LruCache *HelperCache; ///< cache for cert validation helper }; } //namespace Ssl