From 168d2b3069db2d9a53fd7edac2e99caa1f735b64 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 14 Apr 2016 17:30:54 +0300 Subject: [PATCH] Fixes and improvments - Throw a TextException on errors in Ssl::SSL_add_untrusted_cert - Move most of the code from ConnStateData::start() into the new virtual method ConnStateData::prepUserConnection. Call this method instead calling grandparents ::start() method from Downloader::start(). Also implement an empty Downloader::prepUserConnection() method - Fix Downloader::isOpen() to use doneAll() to check if its jobs is finished and its job assumed as closed. - Fix Downloader::start to handle the case a wrong HTTP request passed from the user (eg malformed URL). In this case calaback to the user with an Http::scInternalServerError. - Remove tbe Downloader::callException() method, existed only to print a debug message. Add a debug message in AsyncJob::callException method instead. - Replaces NULL with nullptr - Fixes debug messages and comments --- src/Downloader.cc | 58 ++++++++++++++++---------------------- src/Downloader.h | 24 ++++++++-------- src/base/AsyncJob.cc | 3 +- src/client_side.cc | 9 ++++-- src/client_side.h | 6 +++- src/client_side_request.cc | 4 +-- src/servers/Server.cc | 2 +- src/ssl/PeerConnector.cc | 10 +++---- src/ssl/PeerConnector.h | 12 ++++---- src/ssl/support.cc | 18 ++++++------ 10 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/Downloader.cc b/src/Downloader.cc index 901c5b4a74..0f1dc7be26 100644 --- a/src/Downloader.cc +++ b/src/Downloader.cc @@ -24,13 +24,6 @@ Downloader::~Downloader() debugs(33 , 2, "Downloader Finished"); } -void -Downloader::callException(const std::exception &e) -{ - debugs(33 , 2, "Downloader caught:" << e.what()); - AsyncJob::callException(e); -} - bool Downloader::doneAll() const { @@ -40,39 +33,36 @@ Downloader::doneAll() const void Downloader::start() { - BodyProducer::start(); - HttpControlMsgSink::start(); + ConnStateData::start(); if (Http::Stream *context = parseOneRequest()) { context->registerWithConn(); processParsedRequest(context); - - /**/ if (context->flags.deferred) { if (context != context->http->getConn()->pipeline.front().getRaw()) context->deferRecipientForLater(context->deferredparams.node, context->deferredparams.rep, context->deferredparams.queuedBuffer); else context->http->getConn()->handleReply(context->deferredparams.rep, context->deferredparams.queuedBuffer); } - /**/ - + } else { + status = Http::scInternalServerError; + callBack(); } - } void Downloader::noteMoreBodySpaceAvailable(BodyPipe::Pointer) { - // This method required only if we need to support uploading data to server - // Currently only GET requests are supported - assert(0); + // This method required only if we need to support uploading data to server. + // Currently only GET requests are supported. + assert(false); } void Downloader::noteBodyConsumerAborted(BodyPipe::Pointer) { - // This method required only if we need to support uploading data to server - // Currently only GET requests are supported - assert(0); + // This method required only if we need to support uploading data to server. + // Currently only GET requests are supported. + assert(false); } Http::Stream * @@ -85,7 +75,7 @@ Downloader::parseOneRequest() if (!request) { debugs(33, 5, "Invalid FTP URL: " << uri); safe_free(uri); - return NULL; //earlyError(...) + return nullptr; //earlyError(...) } request->http_ver = Http::ProtocolVersion(); request->header.putStr(Http::HdrType::HOST, request->url.host()); @@ -97,7 +87,7 @@ Downloader::parseOneRequest() http->req_sz = 0; http->uri = uri; - Http::Stream *const context = new Http::Stream(NULL, http); + Http::Stream *const context = new Http::Stream(nullptr, http); StoreIOBuffer tempBuffer; tempBuffer.data = context->reqbuf; tempBuffer.length = HTTP_REQBUF_SZ; @@ -115,14 +105,14 @@ Downloader::parseOneRequest() void Downloader::processParsedRequest(Http::Stream *context) { - Must(context != NULL); + Must(context); Must(pipeline.nrequests == 1); ClientHttpRequest *const http = context->http; - assert(http != NULL); + Must(http); debugs(33, 4, "forwarding request to server side"); - assert(http->storeEntry() == NULL); + Must(http->storeEntry() == nullptr); clientProcessRequest(this, Http1::RequestParserPointer(), context); } @@ -130,13 +120,14 @@ time_t Downloader::idleTimeout() const { // No need to be implemented for connection-less ConnStateData object. - assert(0); + assert(false); return 0; } void Downloader::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) { + // nobody to forward the control message to } void @@ -191,8 +182,8 @@ Downloader::handleReply(HttpReply *reply, StoreIOBuffer receivedData) void Downloader::downloadFinished() { - debugs(33, 3, "fake call, to just delete the Downloader"); - + debugs(33, 7, this); + Must(done()); // Not really needed. Squid will delete this object because "doneAll" is true. //deleteThis("completed"); } @@ -206,19 +197,18 @@ Downloader::callBack() if (status == Http::scOkay) dialer->object = object; ScheduleCallHere(callback); - callback = NULL; + callback = nullptr; // Calling deleteThis method here to finish Downloader // may result to squid crash. // This method called by handleReply method which maybe called - // by ClientHttpRequest::doCallouts. The doCallouts after this object deleted - // may operate on non valid objects. - // Schedule a fake call here just to force squid to delete this object + // by ClientHttpRequest::doCallouts. The doCallouts after this object + // deleted, may operate on non valid objects. + // Schedule a fake call here just to force squid to delete this object. CallJobHere(33, 7, CbcPointer(this), Downloader, downloadFinished); } bool Downloader::isOpen() const { - return cbdataReferenceValid(this) && // XXX: checking "this" in a method - callback != NULL; + return cbdataReferenceValid(this) && !doneAll(); } diff --git a/src/Downloader.h b/src/Downloader.h index be9293fd83..cbb883759f 100644 --- a/src/Downloader.h +++ b/src/Downloader.h @@ -7,12 +7,9 @@ class Downloader: public ConnStateData { CBDATA_CLASS(Downloader); - // XXX CBDATA_CLASS expands to nonvirtual toCbdata, AsyncJob::toCbdata - // is pure virtual. breaks build on clang if override is used - public: - /// Callback data to use with Downloader callbacks; + /// Callback data to use with Downloader callbacks. class CbDialer { public: CbDialer(): status(Http::scNone) {} @@ -21,20 +18,19 @@ public: Http::StatusCode status; }; - explicit Downloader(SBuf &url, const MasterXaction::Pointer &xact, AsyncCall::Pointer &aCallback, unsigned int level = 0); + Downloader(SBuf &url, const MasterXaction::Pointer &xact, AsyncCall::Pointer &aCallback, unsigned int level = 0); virtual ~Downloader(); /// Fake call used internally by Downloader. void downloadFinished(); - /// The nested level of Downloader object (downloads inside downloads) + /// The nested level of Downloader object (downloads inside downloads). unsigned int nestedLevel() const {return level_;} /* ConnStateData API */ virtual bool isOpen() const; /* AsyncJob API */ - virtual void callException(const std::exception &e); virtual bool doneAll() const; /*Bodypipe API*/ @@ -51,19 +47,21 @@ protected: /* AsyncJob API */ virtual void start(); + virtual void prepUserConnection() {}; private: /// Schedules for execution the "callback" with parameters the status - /// and object + /// and object. void callBack(); - static const size_t MaxObjectSize = 1*1024*1024; ///< The maximum allowed object size. + /// The maximum allowed object size. + static const size_t MaxObjectSize = 1*1024*1024; - SBuf url_; ///< The url to download + SBuf url_; ///< the url to download AsyncCall::Pointer callback; ///< callback to call when download finishes - Http::StatusCode status; ///< The download status code - SBuf object; //object data - unsigned int level_; ///< Holds the nested downloads level + Http::StatusCode status; ///< the download status code + SBuf object; ///< the object body data + unsigned int level_; ///< holds the nested downloads level }; #endif diff --git a/src/base/AsyncJob.cc b/src/base/AsyncJob.cc index 874eeaa031..b79edf6425 100644 --- a/src/base/AsyncJob.cc +++ b/src/base/AsyncJob.cc @@ -124,8 +124,9 @@ void AsyncJob::callStart(AsyncCall &call) } void -AsyncJob::callException(const std::exception &) +AsyncJob::callException(const std::exception &ex) { + debugs(93 , 2, ex.what()); // we must be called asynchronously and hence, the caller must lock us Must(cbdataReferenceValid(toCbdata())); diff --git a/src/client_side.cc b/src/client_side.cc index df79f9a5c8..f855e8d4cb 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -586,7 +586,7 @@ ConnStateData::swanSong() debugs(33, 2, HERE << clientConnection); flags.readMore = false; DeregisterRunner(this); - if (clientConnection != NULL) + if (clientConnection != nullptr) clientdbEstablished(clientConnection->remote, -1); /* decrement */ pipeline.terminateAll(0); @@ -2445,7 +2445,7 @@ ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) : pinning.peer = NULL; // store the details required for creating more MasterXaction objects as new requests come in - if (xact->tcpClient != NULL) + if (xact->tcpClient) log_addr = xact->tcpClient->remote; log_addr.applyMask(Config.Addrs.client_netmask); @@ -2460,7 +2460,12 @@ ConnStateData::start() { BodyProducer::start(); HttpControlMsgSink::start(); + prepUserConnection(); +} +void +ConnStateData::prepUserConnection() +{ if (port->disable_pmtu_discovery != DISABLE_PMTU_OFF && (transparent() || port->disable_pmtu_discovery == DISABLE_PMTU_ALWAYS)) { #if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_DONT) diff --git a/src/client_side.h b/src/client_side.h index 1f800b6cbe..4c0cd03691 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -135,7 +135,7 @@ public: /// If the port is not set then it is a connection-less object /// created by an internal squid subsystem - bool connectionless() const { return port == NULL; } + bool connectionless() const { return port == nullptr; } bool transparent() const; @@ -194,6 +194,10 @@ public: virtual bool doneAll() const { return BodyProducer::doneAll() && false;} virtual void swanSong(); + /// Do the related hooks related to start retrieving requests from + /// client connection + virtual void prepUserConnection(); + /// Changes state so that we close the connection and quit after serving /// the client-side-detected error response instead of getting stuck. void quitAfterError(HttpRequest *request); // meant to be private diff --git a/src/client_side_request.cc b/src/client_side_request.cc index dcfc71fb05..c62111f1ea 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -998,8 +998,8 @@ clientCheckPinning(ClientHttpRequest * http) return; // Internal requests such as those from Downloader does not have - // local port - if (http_conn->port == NULL) + // local port. + if (!http_conn->port) return; request->flags.connectionAuthDisabled = http_conn->port->connection_auth_disabled; diff --git a/src/servers/Server.cc b/src/servers/Server.cc index d0fa8461bd..c3b3608ab0 100644 --- a/src/servers/Server.cc +++ b/src/servers/Server.cc @@ -27,7 +27,7 @@ Server::Server(const MasterXaction::Pointer &xact) : port(xact->squidPort), receivedFirstByte_(false) { - if (xact->squidPort != NULL) + if (xact->squidPort) transferProtocol = xact->squidPort->transport; } diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index a82053f07b..d571ca5691 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -352,13 +352,13 @@ Ssl::PeerConnector::noteWantRead() return; // Wait to download certificates before proceed. srvBio->holdRead(false); - // Schedule a negotiateSSl to allow openSSL parse received data + // schedule a negotiateSSl to allow openSSL parse received data Ssl::PeerConnector::NegotiateSsl(fd, this); 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 + // schedule a negotiateSSl to allow openSSL parse received data Ssl::PeerConnector::NegotiateSsl(fd, this); return; } @@ -532,7 +532,7 @@ Ssl::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) certsDownloads++; debugs(81, 5, "Certificate downloading status: " << downloadStatus << " certificate size: " << obj.length()); - // Get ServerBio from SSL object + // get ServerBio from SSL object const int fd = serverConnection()->fd; Security::SessionPtr ssl = fd_table[fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); @@ -550,7 +550,7 @@ Ssl::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) Ssl::SSL_add_untrusted_cert(ssl, cert); } - // Check if has uri to download from and if yes add it to urlsOfMissingCerts + // check if has uri to download from and if yes add it to urlsOfMissingCerts if (urlsOfMissingCerts.size() && certsDownloads <= MaxCertsDownloads) { startCertDownloading(urlsOfMissingCerts.front()); urlsOfMissingCerts.pop(); @@ -566,7 +566,7 @@ Ssl::PeerConnector::checkForMissingCertificates () { // Check for nested SSL certificates downloads. For example when the // certificate located in an SSL site which requires to download a - // a missing certificate (... from an SSL site which requires to ...) + // a missing certificate (... from an SSL site which requires to ...). const Downloader *csd = dynamic_cast(request->clientConnectionManager.valid()); if (csd && csd->nestedLevel() >= MaxNestedDownloads) return false; diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h index ec461dcde8..77404e9add 100644 --- a/src/ssl/PeerConnector.h +++ b/src/ssl/PeerConnector.h @@ -131,10 +131,10 @@ protected: /// issued certificates with Authority Info Access extension. bool checkForMissingCertificates(); - /// Start downloading procedure for the given URL + /// Start downloading procedure for the given URL. void startCertDownloading(SBuf &url); - /// Called by Downloader after a certificate object downloaded + /// Called by Downloader after a certificate object downloaded. void certDownloadingDone(SBuf &object, int status); /// Called when the openSSL SSL_connect function needs to write data to @@ -185,18 +185,18 @@ private: /// A wrapper function for negotiateSsl for use with Comm::SetSelect static void NegotiateSsl(int fd, void *data); - /// The maximum allowed missing certificates downloads + /// The maximum allowed missing certificates downloads. static const unsigned int MaxCertsDownloads = 10; - /// The maximum allowed nested certificates downloads + /// The maximum allowed nested certificates downloads. static const unsigned int MaxNestedDownloads = 3; AsyncCall::Pointer closeHandler; ///< we call this when the connection closed time_t negotiationTimeout; ///< the SSL connection timeout to use time_t startTime; ///< when the peer connector negotiation started bool useCertValidator_; ///< whether the certificate validator should bypassed - /// The list of URLs where missing certificates should be downloaded + /// The list of URLs where missing certificates should be downloaded. std::queue urlsOfMissingCerts; - unsigned int certsDownloads; ///< The number of downloaded missing certificates + unsigned int certsDownloads; ///< the number of downloaded missing certificates }; } // namespace Ssl diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 564da9cfe7..5406dd0a6a 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -1083,10 +1083,10 @@ hasAuthorityInfoAccessCaIssuers(X509 *cert) { AUTHORITY_INFO_ACCESS *info; if (!cert) - return NULL; + return nullptr; info = (AUTHORITY_INFO_ACCESS *)X509_get_ext_d2i(cert, NID_info_access, NULL, NULL); if (!info) - return NULL; + return nullptr; static char uri[MAX_URL]; uri[0] = '\0'; @@ -1101,7 +1101,7 @@ hasAuthorityInfoAccessCaIssuers(X509 *cert) } } AUTHORITY_INFO_ACCESS_free(info); - return uri[0] != '\0' ? uri : NULL; + return uri[0] != '\0' ? uri : nullptr; } bool @@ -1165,20 +1165,20 @@ const char * Ssl::uriOfIssuerIfMissing(X509 *cert, Ssl::X509_STACK_Pointer const &serverCertificates) { if (!cert || !serverCertificates.get()) - return NULL; + return nullptr; if (!findCertByIssuerSlowly(serverCertificates.get(), cert)) { - //if issuer is missing ... + //if issuer is missing if (!findCertByIssuerFast(SquidUntrustedCerts, cert)) { // and issuer not found in local untrusted certificates database if (const char *issuerUri = hasAuthorityInfoAccessCaIssuers(cert)) { // There is a URI where we can download a certificate. - // Check to see if this is required + // Check to see if this is required. return issuerUri; } } } - return NULL; + return nullptr; } void @@ -1201,8 +1201,8 @@ Ssl::SSL_add_untrusted_cert(SSL *ssl, X509 *cert) if (!untrustedStack) { untrustedStack = sk_X509_new_null(); if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_untrusted_chain, untrustedStack)) { - sk_X509_pop_free(untrustedStack, X509_free); //?? print an error? - return; + sk_X509_pop_free(untrustedStack, X509_free); + throw TextException("Failed to attach untrusted certificates chain"); } } sk_X509_push(untrustedStack, cert); -- 2.47.2