From: Christos Tsantilas Date: Fri, 15 Jul 2016 11:05:34 +0000 (+0300) Subject: Squid-dev review comments by Alex and Amos X-Git-Tag: SQUID_4_0_13~5^2~1^2~3 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4b5ea8a6d4d809deb75cf275ca963baa5d9a73b0;p=thirdparty%2Fsquid.git Squid-dev review comments by Alex and Amos --- diff --git a/src/Downloader.cc b/src/Downloader.cc index 901b2559a8..cdad7d9db6 100644 --- a/src/Downloader.cc +++ b/src/Downloader.cc @@ -1,3 +1,11 @@ +/* + * Copyright (C) 1996-2016 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + #include "squid.h" #include "client_side.h" #include "client_side_request.h" @@ -7,33 +15,36 @@ #include "http/one/RequestParser.h" #include "http/Stream.h" +CBDATA_CLASS_INIT(Downloader); + /// Used to hold and pass the required info and buffers to the /// clientStream callbacks class DownloaderContext: public RefCountable { - CBDATA_CLASS(DownloaderContext); + MEMPROXY_CLASS(DownloaderContext); public: typedef RefCount Pointer; - DownloaderContext(Downloader *dl, ClientHttpRequest *h): - downloader(cbdataReference(dl)), - http(h) - {} + DownloaderContext(Downloader *dl, ClientHttpRequest *h); ~DownloaderContext(); void finished(); - Downloader* downloader; + + CbcPointer downloader; ClientHttpRequest *http; char requestBuffer[HTTP_REQBUF_SZ]; }; -CBDATA_CLASS_INIT(DownloaderContext); -CBDATA_CLASS_INIT(Downloader); +DownloaderContext::DownloaderContext(Downloader *dl, ClientHttpRequest *h): + downloader(dl), + http(h) +{ + debugs(33, 6, "DownloaderContext constructed, this=" << (void*)this); +} DownloaderContext::~DownloaderContext() { - debugs(33, 5, HERE); - cbdataReferenceDone(downloader); + debugs(33, 6, "DownloaderContext destructed, this=" << (void*)this); if (http) finished(); } @@ -42,36 +53,34 @@ void DownloaderContext::finished() { delete http; - http = NULL; + http = nullptr; } Downloader::Downloader(SBuf &url, AsyncCall::Pointer &aCallback, unsigned int level): AsyncJob("Downloader"), url_(url), - callback(aCallback), - status(Http::scNone), + callback_(aCallback), level_(level) { } Downloader::~Downloader() { - debugs(33 , 2, HERE); } bool Downloader::doneAll() const { - return (!callback || callback->canceled()) && AsyncJob::doneAll(); + return (!callback_ || callback_->canceled()) && AsyncJob::doneAll(); } static void downloaderRecipient(clientStreamNode * node, ClientHttpRequest * http, HttpReply * rep, StoreIOBuffer receivedData) { - debugs(33, 6, HERE); + debugs(33, 6, MYNAME); /* Test preconditions */ - assert(node != NULL); + assert(node); /* TODO: handle this rather than asserting * - it should only ever happen if we cause an abort and @@ -79,33 +88,32 @@ downloaderRecipient(clientStreamNode * node, ClientHttpRequest * http, * However, that itself shouldn't happen, so it stays as an assert for now. */ assert(cbdataReferenceValid(node)); - assert(node->node.next == NULL); + assert(!node->node.next); DownloaderContext::Pointer context = dynamic_cast(node->data.getRaw()); - assert(context != NULL); + assert(context); - if (!cbdataReferenceValid(context->downloader)) - return; - - context->downloader->handleReply(node, http, rep, receivedData); + if (context->downloader.valid()) + context->downloader->handleReply(node, http, rep, receivedData); } static void downloaderDetach(clientStreamNode * node, ClientHttpRequest * http) { - debugs(33, 5, HERE); + debugs(33, 5, MYNAME); clientStreamDetach(node, http); } +/// Initializes and starts the HTTP GET request to the remote server bool Downloader::buildRequest() { const HttpRequestMethod method = Http::METHOD_GET; - char *uri = strdup(url_.c_str()); + char *uri = xstrdup(url_.c_str()); HttpRequest *const request = HttpRequest::CreateFromUrl(uri, method); if (!request) { - debugs(33, 5, "Invalid FTP URL: " << uri); - safe_free(uri); + debugs(33, 5, "Invalid URI: " << url_); + xfree(uri); return false; //earlyError(...) } request->http_ver = Http::ProtocolVersion(); @@ -120,7 +128,12 @@ Downloader::buildRequest() request->my_addr.port(0); request->downloader = this; - ClientHttpRequest *const http = new ClientHttpRequest(NULL); + debugs(11, 2, "HTTP Client Downloader " << this << "/" << id); + debugs(11, 2, "HTTP Client REQUEST:\n---------\n" << + request->method << " " << url_ << " " << request->http_ver << "\n" << + "\n----------"); + + ClientHttpRequest *const http = new ClientHttpRequest(nullptr); http->request = request; HTTPMSGLOCK(http->request); http->req_sz = 0; @@ -139,14 +152,6 @@ Downloader::buildRequest() // Build a ClientRequestContext to start doCallouts http->calloutContext = new ClientRequestContext(http); - - // Do not check for redirect, tos,nfmark and sslBump - http->calloutContext->redirect_done = true; - http->calloutContext->tosToClientDone = true; - http->calloutContext->nfmarkToClientDone = true; - http->calloutContext->sslBumpCheckDone = true; - http->al->ssl.bumpMode = Ssl::bumpEnd; // SslBump does not apply; log - - http->doCallouts(); return true; } @@ -154,63 +159,65 @@ Downloader::buildRequest() void Downloader::start() { - if (!buildRequest()) { - status = Http::scInternalServerError; - callBack(); - } + if (!buildRequest()) + callBack(Http::scInternalServerError); } void Downloader::handleReply(clientStreamNode * node, ClientHttpRequest *http, HttpReply *reply, StoreIOBuffer receivedData) { - // TODO: remove the following check: DownloaderContext::Pointer callerContext = dynamic_cast(node->data.getRaw()); + // TODO: remove the following check: assert(callerContext == context_); - bool existingContent = reply ? reply->content_length : 0; - bool exceedSize = (existingContent > -1 && (size_t)existingContent > MaxObjectSize) || - ((object.length() + receivedData.length) > MaxObjectSize); + debugs(33, 4, "Received " << receivedData.length << + " object data, offset: " << receivedData.offset << + " error flag:" << receivedData.flags.error); - if (exceedSize) { - status = Http::scInternalServerError; - callBack(); + const bool failed = receivedData.flags.error; + if (failed) { + callBack(Http::scInternalServerError); return; } - debugs(33, 4, "Received " << receivedData.length << - " object data, offset: " << receivedData.offset << - " error flag:" << receivedData.flags.error); + const int64_t existingContent = reply ? reply->content_length : 0; + const size_t maxSize = MaxObjectSize > SBuf::maxSize ? SBuf::maxSize : MaxObjectSize; + const bool tooLarge = (existingContent > -1 && existingContent > static_cast(maxSize)) || + (maxSize < object_.length()) || + ((maxSize - object_.length()) < receivedData.length); + + if (tooLarge) { + callBack(Http::scInternalServerError); + return; + } - if (receivedData.length > 0) { - object.append(receivedData.data, receivedData.length); + if (receivedData.length) { + object_.append(receivedData.data, receivedData.length); http->out.size += receivedData.length; http->out.offset += receivedData.length; } - switch (clientStreamStatus (node, http)) { + switch (clientStreamStatus(node, http)) { case STREAM_NONE: { - debugs(33, 3, HERE << "Get more data"); + debugs(33, 3, "Get more data"); StoreIOBuffer tempBuffer; tempBuffer.offset = http->out.offset; tempBuffer.data = context_->requestBuffer; tempBuffer.length = HTTP_REQBUF_SZ; - clientStreamRead (node, http, tempBuffer); + clientStreamRead(node, http, tempBuffer); } break; case STREAM_COMPLETE: - debugs(33, 3, HERE << "Object data transfer successfully complete"); - status = Http::scOkay; - callBack(); + debugs(33, 3, "Object data transfer successfully complete"); + callBack(Http::scOkay); break; case STREAM_UNPLANNED_COMPLETE: - debugs(33, 3, HERE << "Object data transfer failed: STREAM_UNPLANNED_COMPLETE"); - status = Http::scInternalServerError; - callBack(); + debugs(33, 3, "Object data transfer failed: STREAM_UNPLANNED_COMPLETE"); + callBack(Http::scInternalServerError); break; case STREAM_FAILED: - debugs(33, 3, HERE << "Object data transfer failed: STREAM_FAILED"); - status = Http::scInternalServerError; - callBack(); + debugs(33, 3, "Object data transfer failed: STREAM_FAILED"); + callBack(Http::scInternalServerError); break; default: fatal("unreachable code"); @@ -221,29 +228,34 @@ void Downloader::downloadFinished() { debugs(33, 7, this); + // We cannot delay http destruction until refcounting deletes + // DownloaderContext. The http object destruction will cause + // clientStream cleanup and will release the refcount to context_ + // object hold by clientStream structures. context_->finished(); - context_ = NULL; + context_ = nullptr; Must(done()); - // Not really needed. Squid will delete this object because "doneAll" is true. - //deleteThis("completed"); } +/// Schedules for execution the "callback" with parameters the status +/// and object. void -Downloader::callBack() +Downloader::callBack(Http::StatusCode const statusCode) { - CbDialer *dialer = dynamic_cast(callback->getDialer()); + CbDialer *dialer = dynamic_cast(callback_->getDialer()); Must(dialer); - dialer->status = status; - if (status == Http::scOkay) - dialer->object = object; - ScheduleCallHere(callback); - callback = nullptr; + dialer->status = statusCode; + if (statusCode == Http::scOkay) + dialer->object = object_; + ScheduleCallHere(callback_); + 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. + // Schedule an async call here just to force squid to delete this object. CallJobHere(33, 7, CbcPointer(this), Downloader, downloadFinished); } diff --git a/src/Downloader.h b/src/Downloader.h index 4d6546252d..b409dfa0ff 100644 --- a/src/Downloader.h +++ b/src/Downloader.h @@ -1,17 +1,23 @@ +/* + * Copyright (C) 1996-2016 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + #ifndef SQUID_DOWNLOADER_H #define SQUID_DOWNLOADER_H -#include "base/AsyncCall.h" #include "base/AsyncJob.h" -#include "cbdata.h" #include "defines.h" +#include "http/forward.h" #include "http/StatusCode.h" #include "sbuf/SBuf.h" class ClientHttpRequest; class StoreIOBuffer; class clientStreamNode; -class HttpReply; class DownloaderContext; typedef RefCount DownloaderContextPointer; @@ -36,37 +42,31 @@ public: Downloader(SBuf &url, AsyncCall::Pointer &aCallback, unsigned int level = 0); virtual ~Downloader(); - /// Fake call used internally by Downloader. + /// delays destruction to protect doCallouts() void downloadFinished(); /// The nested level of Downloader object (downloads inside downloads). unsigned int nestedLevel() const {return level_;} - /* AsyncJob API */ - virtual bool doneAll() const; + void handleReply(clientStreamNode *, ClientHttpRequest *, HttpReply *, StoreIOBuffer); - void handleReply(clientStreamNode * node, ClientHttpRequest *http, HttpReply *header, StoreIOBuffer receivedData); protected: /* AsyncJob API */ + virtual bool doneAll() const; virtual void start(); private: - /// Initializes and starts the HTTP GET request to the remote server bool buildRequest(); - - /// Schedules for execution the "callback" with parameters the status - /// and object. - void callBack(); + void callBack(Http::StatusCode const status); /// The maximum allowed object size. static const size_t MaxObjectSize = 1*1024*1024; SBuf url_; ///< the url to download - AsyncCall::Pointer callback; ///< callback to call when download finishes - Http::StatusCode status; ///< the download status code - SBuf object; ///< the object body data + AsyncCall::Pointer callback_; ///< callback to call when download finishes + SBuf object_; ///< the object body data const unsigned int level_; ///< holds the nested downloads level /// Pointer to an object that stores the clientStream required info diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 66422d1fd8..8d9702ac58 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -213,7 +213,7 @@ public: */ CbcPointer clientConnectionManager; - /// The Downloader object intiated the HTTP request if exist + /// The Downloader object which initiated the HTTP request if any CbcPointer downloader; /// forgets about the cached Range header (for a reason) diff --git a/src/Makefile.am b/src/Makefile.am index 8408005aa1..acdfd99073 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -272,8 +272,8 @@ squid_SOURCES = \ dlink.h \ dlink.cc \ $(DNSSOURCE) \ - Downloader.h \ Downloader.cc \ + Downloader.h \ enums.h \ err_type.h \ err_detail_type.h \ diff --git a/src/base/AsyncJob.cc b/src/base/AsyncJob.cc index b79edf6425..96f110183a 100644 --- a/src/base/AsyncJob.cc +++ b/src/base/AsyncJob.cc @@ -126,7 +126,7 @@ void AsyncJob::callStart(AsyncCall &call) void AsyncJob::callException(const std::exception &ex) { - debugs(93 , 2, ex.what()); + 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_reply.cc b/src/client_side_reply.cc index ccd279ebd6..eccb6b5622 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -2131,7 +2131,7 @@ clientReplyContext::sendMoreData (StoreIOBuffer result) return; } - if (reqofs==0 && !http->logType.isTcpHit() && Comm::IsConnOpen(conn->clientConnection)) { + if (reqofs==0 && !http->logType.isTcpHit()) { if (Ip::Qos::TheConfig.isHitTosActive()) { Ip::Qos::doTosLocalMiss(conn->clientConnection, http->request->hier.code); } @@ -2140,8 +2140,7 @@ clientReplyContext::sendMoreData (StoreIOBuffer result) } } - debugs(88, 5, "clientReplyContext::sendMoreData:" << - conn->clientConnection << + debugs(88, 5, conn->clientConnection << " '" << entry->url() << "'" << " out.offset=" << http->out.offset); } diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 87e9aa7c88..f269b533ec 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1409,6 +1409,11 @@ ClientRequestContext::checkNoCacheDone(const allow_t &answer) bool ClientRequestContext::sslBumpAccessCheck() { + if (!http->getConn()) { + http->al->ssl.bumpMode = Ssl::bumpEnd; // SslBump does not apply; log - + return false; + } + // If SSL connection tunneling or bumping decision has been made, obey it. const Ssl::BumpMode bumpMode = http->getConn()->sslBumpMode; if (bumpMode != Ssl::bumpEnd) { diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc index 7dfc7cc04f..c664f41f48 100644 --- a/src/security/Handshake.cc +++ b/src/security/Handshake.cc @@ -539,14 +539,12 @@ void Security::HandshakeParser::ParseCertificate(const SBuf &raw, Security::CertPointer &pCert) { #if USE_OPENSSL - typedef const unsigned char *x509Data; - const x509Data x509Start = reinterpret_cast(raw.rawContent()); - x509Data x509Pos = x509Start; + auto x509Start = reinterpret_cast(raw.rawContent()); + auto x509Pos = x509Start; X509 *x509 = d2i_X509(nullptr, &x509Pos, raw.length()); Must(x509); // successfully parsed Must(x509Pos == x509Start + raw.length()); // no leftovers pCert.resetAndLock(x509); -#else #endif } diff --git a/src/security/forward.h b/src/security/forward.h index e14f475a0f..70e03e22d6 100644 --- a/src/security/forward.h +++ b/src/security/forward.h @@ -56,10 +56,10 @@ typedef Security::LockingPointer CertRevokeList; - typedef std::list CertList; +typedef std::list CertRevokeList; + #if USE_OPENSSL CtoCpp1(DH_free, DH *); typedef Security::LockingPointer DhePointer; diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index abdf5cfc57..7efdc25ace 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -550,7 +550,7 @@ Ssl::PeerConnector::startCertDownloading(SBuf &url) void Ssl::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) { - certsDownloads++; + ++certsDownloads; debugs(81, 5, "Certificate downloading status: " << downloadStatus << " certificate size: " << obj.length()); // get ServerBio from SSL object @@ -571,7 +571,8 @@ 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 there are URIs to download from and if yes start downloading + // the first in queue. if (urlsOfMissingCerts.size() && certsDownloads <= MaxCertsDownloads) { startCertDownloading(urlsOfMissingCerts.front()); urlsOfMissingCerts.pop(); @@ -583,7 +584,7 @@ Ssl::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) } bool -Ssl::PeerConnector::checkForMissingCertificates () +Ssl::PeerConnector::checkForMissingCertificates() { // Check for nested SSL certificates downloads. For example when the // certificate located in an SSL site which requires to download a @@ -591,7 +592,7 @@ Ssl::PeerConnector::checkForMissingCertificates () // XXX: find a way to link HttpRequest with Downloader. // The following always fails: - const Downloader *csd = dynamic_cast(request->downloader.valid()); + const Downloader *csd = request->downloader.get(); if (csd && csd->nestedLevel() >= MaxNestedDownloads) return false; diff --git a/src/ssl/bio.h b/src/ssl/bio.h index ef4d0ba7af..f5612aefaa 100644 --- a/src/ssl/bio.h +++ b/src/ssl/bio.h @@ -152,12 +152,13 @@ public: void mode(Ssl::BumpMode m) {bumpMode_ = m;} Ssl::BumpMode bumpMode() {return bumpMode_;} ///< return the bumping mode - /// Return true if the Server hello message received + /// \retval true if the Server hello message received bool gotHello() const { return (parsedHandshake && !parseError); } /// Return true if the Server Hello parsing failed bool gotHelloFailed() const { return (parsedHandshake && parseError); } + /// \return the server certificates list if received and parsed correctly const Security::CertList &serverCertificatesIfAny() { return parser_.serverCertificates; } /// \return the TLS Details advertised by TLS server. diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 1bb7debe2b..f064bad23d 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -1100,7 +1100,7 @@ hasAuthorityInfoAccessCaIssuers(X509 *cert) AUTHORITY_INFO_ACCESS *info; if (!cert) return nullptr; - info = (AUTHORITY_INFO_ACCESS *)X509_get_ext_d2i(cert, NID_info_access, NULL, NULL); + info = static_cast(X509_get_ext_d2i(cert, NID_info_access, NULL, NULL)); if (!info) return nullptr; @@ -1111,7 +1111,7 @@ hasAuthorityInfoAccessCaIssuers(X509 *cert) ACCESS_DESCRIPTION *ad = sk_ACCESS_DESCRIPTION_value(info, i); if (OBJ_obj2nid(ad->method) == NID_ad_ca_issuers) { if (ad->location->type == GEN_URI) { - xstrncpy(uri, (char *)ASN1_STRING_data(ad->location->d.uniformResourceIdentifier), sizeof(uri)); + xstrncpy(uri, reinterpret_cast(ASN1_STRING_data(ad->location->d.uniformResourceIdentifier)), sizeof(uri)); } break; } @@ -1185,7 +1185,6 @@ Ssl::uriOfIssuerIfMissing(X509 *cert, Security::CertList const &serverCertificat // 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. return issuerUri; } } @@ -1199,8 +1198,8 @@ Ssl::missingChainCertificatesUrls(std::queue &URIs, Security::CertList con if (!serverCertificates.size()) return; - for (Security::CertList::const_iterator it = serverCertificates.begin(); it != serverCertificates.end(); ++it) { - if (const char *issuerUri = uriOfIssuerIfMissing(it->get(), serverCertificates)) + for (const auto &i : serverCertificates) { + if (const char *issuerUri = uriOfIssuerIfMissing(i.get(), serverCertificates)) URIs.push(SBuf(issuerUri)); } } @@ -1270,7 +1269,7 @@ untrustedToStoreCtx_cb(X509_STORE_CTX *ctx,void *data) { debugs(83, 4, "Try to use pre-downloaded intermediate certificates\n"); - SSL *ssl = (SSL *)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + SSL *ssl = static_cast(X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx())); STACK_OF(X509) *sslUntrustedStack = static_cast (SSL_get_ex_data(ssl, ssl_ex_index_ssl_untrusted_chain)); // OpenSSL already maintains ctx->untrusted but we cannot modify diff --git a/src/ssl/support.h b/src/ssl/support.h index dec887acb5..bbe655adea 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -187,27 +187,23 @@ inline const char *bumpMode(int bm) typedef std::multimap CertsIndexedList; /** - \ingroup ServerProtocolSSLAPI * Load PEM-encoded certificates from the given file. */ bool loadCerts(const char *certsFile, Ssl::CertsIndexedList &list); /** - \ingroup ServerProtocolSSLAPI * Load PEM-encoded certificates to the squid untrusteds certificates * internal DB from the given file. */ bool loadSquidUntrusted(const char *path); /** - \ingroup ServerProtocolSSLAPI * Removes all certificates from squid untrusteds certificates * internal DB and frees all memory */ void unloadSquidUntrusted(); /** - \ingroup ServerProtocolSSLAPI * Add the certificate cert to ssl object untrusted certificates. * Squid uses an attached to SSL object list of untrusted certificates, * with certificates which can be used to complete incomplete chains sent @@ -216,14 +212,12 @@ void unloadSquidUntrusted(); void SSL_add_untrusted_cert(SSL *ssl, X509 *cert); /** - \ingroup ServerProtocolSSLAPI * Searches in serverCertificates list for the cert issuer and if not found * and Authority Info Access of cert provides a URI return it. */ const char *uriOfIssuerIfMissing(X509 *cert, Security::CertList const &serverCertificates); /** - \ingroup ServerProtocolSSLAPI * Fill URIs queue with the uris of missing certificates from serverCertificate chain * if this information provided by Authority Info Access. */ @@ -325,13 +319,6 @@ void addChainToSslContext(Security::ContextPtr sslContext, STACK_OF(X509) *certL */ void useSquidUntrusted(SSL_CTX *sslContext); -/** - \ingroup ServerProtocolSSLAPI - * Configures sslContext to use squid untrusted certificates internal list - * to complete certificate chains when verifies SSL servers certificates. - */ -void useSquidUntrusted(SSL_CTX *sslContext); - /** \ingroup ServerProtocolSSLAPI * Read certificate, private key and any certificates which must be chained from files.