------------------------------------------------------------ revno: 13967 revision-id: squid3@treenet.co.nz-20151222081159-rz19bo6w7b2n5l2p parent: squid3@treenet.co.nz-20151222075423-acprzy6amd0zj1yx author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Tue 2015-12-22 21:11:59 +1300 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20151222081159-rz19bo6w7b2n5l2p # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: f074ae493ecb902754ac023e7485c2309acf0cef # timestamp: 2015-12-22 08:20:53 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20151222075423-\ # acprzy6amd0zj1yx # # Begin patch === modified file 'src/ssl/PeerConnector.cc' --- src/ssl/PeerConnector.cc 2015-09-27 08:18:53 +0000 +++ src/ssl/PeerConnector.cc 2015-12-22 08:11:59 +0000 @@ -342,7 +342,8 @@ 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 @@ } 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 @@ 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) { === modified file 'src/ssl/PeerConnector.h' --- src/ssl/PeerConnector.h 2015-09-27 08:18:53 +0000 +++ src/ssl/PeerConnector.h 2015-12-22 08:11:59 +0000 @@ -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 @@ 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 @@ 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 @@ /// 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); === modified file 'src/ssl/cert_validate_message.h' --- src/ssl/cert_validate_message.h 2015-01-13 09:13:49 +0000 +++ src/ssl/cert_validate_message.h 2015-12-22 08:11:59 +0000 @@ -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 @@ * 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. === modified file 'src/ssl/helper.cc' --- src/ssl/helper.cc 2015-05-22 04:55:35 +0000 +++ src/ssl/helper.cc 2015-12-22 08:11:59 +0000 @@ -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 @@ //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 @@ struct submitData { std::string query; - Ssl::CertValidationHelper::CVHCB *callback; - void *data; + AsyncCall::Pointer callback; SSL *ssl; CBDATA_CLASS2(submitData); }; @@ -218,7 +217,7 @@ 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 @@ } 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 @@ 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 @@ 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; === modified file 'src/ssl/helper.h' --- src/ssl/helper.h 2015-01-13 09:13:49 +0000 +++ src/ssl/helper.h 2015-12-22 08:11:59 +0000 @@ -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 @@ }; #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