From: Christos Tsantilas Date: Tue, 22 Dec 2015 08:11:59 +0000 (+1300) Subject: Avoid memory leaks when a certificate validator is used with SslBump X-Git-Tag: SQUID_3_5_13~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2f909411f73291769d5f73b656f74279dc7baa1b;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 bc88cf6b87..df375c4cb0 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -342,7 +342,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, NULL)); + Ssl::CertValidationHelper::GetInstance()->sslSubmit(validationRequest, call); return false; } catch (const std::exception &e) { debugs(83, DBG_IMPORTANT, "ERROR: Failed to compose ssl_crtvd " << @@ -465,15 +466,10 @@ Ssl::PeerConnector::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; @@ -481,11 +477,11 @@ Ssl::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse const &valid return; } - debugs(83,5, request->GetHost() << " cert validation result: " << validationResponse.resultCode); + debugs(83,5, request->GetHost() << " 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 44e95c08d1..ffde4265a9 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 "ssl/support.h" #include @@ -23,6 +24,7 @@ namespace Ssl class ErrorDetail; class CertValidationResponse; +typedef RefCount CertValidationResponsePointer; /// PeerConnector results (supplied via a callback). /// The connection to peer was secured if and only if the error member is nil. @@ -154,7 +156,7 @@ 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 *&); @@ -167,9 +169,6 @@ private: /// connection manager members void serverCertificateVerified(); - /// 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); diff --git a/src/ssl/cert_validate_message.h b/src/ssl/cert_validate_message.h index 79edea4ab5..8e13e2333d 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 30892d5aa8..5d2e377c4a 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -19,7 +19,7 @@ #include "SwapDir.h" #include "wordlist.h" -LruMap *Ssl::CertValidationHelper::HelperCache = NULL; +Ssl::CertValidationHelper::LruCache *Ssl::CertValidationHelper::HelperCache = NULL; #if USE_SSL_CRTD Ssl::Helper * Ssl::Helper::GetInstance() @@ -186,7 +186,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() @@ -207,8 +207,7 @@ void Ssl::CertValidationHelper::Shutdown() struct submitData { std::string query; - Ssl::CertValidationHelper::CVHCB *callback; - void *data; + AsyncCall::Pointer callback; SSL *ssl; CBDATA_CLASS2(submitData); }; @@ -218,7 +217,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); @@ -234,20 +233,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) { static time_t first_warn = 0; assert(ssl_crt_validator); @@ -258,9 +259,11 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ if (squid_curtime - first_warn > 3 * 60) fatal("ssl_crtvd queue being overloaded for long time"); debugs(83, DBG_IMPORTANT, "WARNING: ssl_crtvd queue overload, rejecting"); - 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); return; } first_warn = 0; @@ -274,15 +277,16 @@ 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; 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