From 8babada05a107f0755510e6b40cdde309fb48008 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sun, 9 Jul 2017 20:16:33 +1200 Subject: [PATCH] Bug 1961 extra: Convert the URL::parse method API to take const URI strings The input buffer is no longer truncated when overly long. All callers have been checked that they handle the bool false return value in ways that do not rely on that truncation. Callers that were making non-const copies of buffers specifically for the parsing stage are altered not to do so. This allows a few data copies and allocations to be removed entirely, or delayed to remove from error handling paths. While checking all the callers of Http::FromUrl several places were found to be using the "raw" URL string before the parsing and validation was done. The simplest in src/mime.cc is already applied to v5 r15234. A more complicated redesign in src/store_digest.cc is included here for review. One other marked with an "XXX: polluting ..." note. Also, added several TODO's to mark code where class URL needs to be used when the parser is a bit more efficient. Also, removed a leftover definition of already removed urlParse() function. --- src/Downloader.cc | 7 +++---- src/HttpRequest.cc | 4 ++-- src/HttpRequest.h | 2 +- src/URL.h | 3 +-- src/acl/Asn.cc | 1 + src/adaptation/ecap/MessageRep.cc | 4 +--- src/client_side_request.cc | 4 ++-- src/mgr/Inquirer.cc | 3 +-- src/neighbors.cc | 5 +++-- src/redirect.cc | 4 ---- src/servers/FtpServer.cc | 6 +++--- src/servers/Http1Server.cc | 1 + src/store_digest.cc | 20 ++++++++++---------- src/tests/stub_HttpRequest.cc | 2 +- src/tests/stub_url.cc | 2 +- src/url.cc | 4 +--- src/urn.cc | 20 +++++++++----------- 17 files changed, 41 insertions(+), 51 deletions(-) diff --git a/src/Downloader.cc b/src/Downloader.cc index e153861d97..ccfe998e8b 100644 --- a/src/Downloader.cc +++ b/src/Downloader.cc @@ -128,12 +128,10 @@ Downloader::buildRequest() { const HttpRequestMethod method = Http::METHOD_GET; - char *uri = xstrdup(url_.c_str()); const MasterXaction::Pointer mx = new MasterXaction(initiator_); - HttpRequest *const request = HttpRequest::FromUrl(uri, mx, method); + HttpRequest *const request = HttpRequest::FromUrl(url_.c_str(), mx, method); if (!request) { debugs(33, 5, "Invalid URI: " << url_); - xfree(uri); return false; //earlyError(...) } request->http_ver = Http::ProtocolVersion(); @@ -156,7 +154,8 @@ Downloader::buildRequest() http->request = request; HTTPMSGLOCK(http->request); http->req_sz = 0; - http->uri = uri; + // XXX: performance regression. c_str() reallocates + http->uri = xstrdup(url_.c_str()); setLogUri (http, urlCanonicalClean(request)); context_ = new DownloaderContext(this, http); diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 4366f66be1..509d2a44df 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -332,7 +332,7 @@ HttpRequest::parseFirstLine(const char *start, const char *end) * (char *) end = '\0'; // temp terminate URI, XXX dangerous? - const bool ret = url.parse(method, (char *) start); + const bool ret = url.parse(method, start); * (char *) end = save; @@ -520,7 +520,7 @@ HttpRequest::expectingBody(const HttpRequestMethod &, int64_t &theSize) const * If the request cannot be created cleanly, NULL is returned */ HttpRequest * -HttpRequest::FromUrl(char * url, const MasterXaction::Pointer &mx, const HttpRequestMethod& method) +HttpRequest::FromUrl(const char * url, const MasterXaction::Pointer &mx, const HttpRequestMethod& method) { std::unique_ptr req(new HttpRequest(mx)); if (req->url.parse(method, url)) { diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 0d8b2ca8f2..dd22412449 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -199,7 +199,7 @@ public: static void httpRequestPack(void *obj, Packable *p); - static HttpRequest * FromUrl(char * url, const MasterXaction::Pointer &, const HttpRequestMethod &method = Http::METHOD_GET); + static HttpRequest * FromUrl(const char * url, const MasterXaction::Pointer &, const HttpRequestMethod &method = Http::METHOD_GET); ConnStateData *pinnedConnection(); diff --git a/src/URL.h b/src/URL.h index 88e856698c..ccf8d62c45 100644 --- a/src/URL.h +++ b/src/URL.h @@ -53,7 +53,7 @@ public: } void touch(); ///< clear the cached URI display forms - bool parse(const HttpRequestMethod &, char *url); + bool parse(const HttpRequestMethod &, const char *url); AnyP::UriScheme const & getScheme() const {return scheme_;} @@ -170,7 +170,6 @@ class HttpRequest; class HttpRequestMethod; void urlInitialize(void); -bool urlParse(const HttpRequestMethod&, char *, HttpRequest &request); char *urlCanonicalClean(const HttpRequest *); const char *urlCanonicalFakeHttps(const HttpRequest * request); bool urlIsRelative(const char *); diff --git a/src/acl/Asn.cc b/src/acl/Asn.cc index c152f29bf3..c2e23bbe27 100644 --- a/src/acl/Asn.cc +++ b/src/acl/Asn.cc @@ -235,6 +235,7 @@ asnStats(StoreEntry * sentry) static void asnCacheStart(int as) { + // TODO: use class URL instead of generating a string and re-parsing LOCAL_ARRAY(char, asres, 4096); StoreEntry *e; ASState *asState = new ASState; diff --git a/src/adaptation/ecap/MessageRep.cc b/src/adaptation/ecap/MessageRep.cc index 31e37ba3cb..ac02172e8f 100644 --- a/src/adaptation/ecap/MessageRep.cc +++ b/src/adaptation/ecap/MessageRep.cc @@ -205,10 +205,8 @@ Adaptation::Ecap::RequestLineRep::uri(const Area &aUri) { // TODO: if method is not set, URL::parse will assume it is not connect; // Can we change URL::parse API to remove the method parameter? - // TODO: optimize: URL::parse should take constant URL buffer - char *buf = xstrdup(aUri.toString().c_str()); + const char *buf = aUri.toString().c_str(); const bool ok = theMessage.url.parse(theMessage.method, buf); - xfree(buf); Must(ok); } diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 664ddf0e1d..6d18f02a15 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -344,7 +344,7 @@ clientBeginRequest(const HttpRequestMethod& method, char const *url, CSCB * stre /* allow size for url rewriting */ url_sz = strlen(url) + Config.appendDomainLen + 5; http->uri = (char *)xcalloc(url_sz, 1); - strcpy(http->uri, url); + strcpy(http->uri, url); // XXX: polluting http->uri before parser validation if ((request = HttpRequest::FromUrl(http->uri, mx, method)) == NULL) { debugs(85, 5, "Invalid URL: " << http->uri); @@ -1265,7 +1265,7 @@ ClientRequestContext::clientRedirectDone(const Helper::Reply &reply) // prevent broken helpers causing too much damage. If old URL == new URL skip the re-write. if (urlNote != NULL && strcmp(urlNote, http->uri)) { URL tmpUrl; - if (tmpUrl.parse(old_request->method, const_cast(urlNote))) { + if (tmpUrl.parse(old_request->method, urlNote)) { HttpRequest *new_request = old_request->clone(); new_request->url = tmpUrl; debugs(61, 2, "URL-rewriter diverts URL from " << old_request->effectiveRequestUri() << " to " << new_request->effectiveRequestUri()); diff --git a/src/mgr/Inquirer.cc b/src/mgr/Inquirer.cc index fc662d8bf5..92b134ae26 100644 --- a/src/mgr/Inquirer.cc +++ b/src/mgr/Inquirer.cc @@ -74,8 +74,7 @@ Mgr::Inquirer::start() std::unique_ptr replyBuf; if (strands.empty()) { - LOCAL_ARRAY(char, url, MAX_URL); - snprintf(url, MAX_URL, "%s", aggrAction->command().params.httpUri.termedBuf()); + const char *url = aggrAction->command().params.httpUri.termedBuf(); const MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initIpc); HttpRequest *req = HttpRequest::FromUrl(url, mx); ErrorState err(ERR_INVALID_URL, Http::scNotFound, req); diff --git a/src/neighbors.cc b/src/neighbors.cc index 65ae2ef590..afb245ade3 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -1373,19 +1373,20 @@ peerCountMcastPeersStart(void *data) // APIs) to pass around a few basic data points like start_ping and ping! CachePeer *p = (CachePeer *)data; ps_state *psstate; - StoreEntry *fake; MemObject *mem; icp_common_t *query; int reqnum; + // TODO: use class URL instead of constructing and re-parsing a string LOCAL_ARRAY(char, url, MAX_URL); assert(p->type == PEER_MULTICAST); p->mcast.flags.count_event_pending = false; snprintf(url, MAX_URL, "http://"); p->in_addr.toUrl(url+7, MAX_URL -8 ); strcat(url, "/"); - fake = storeCreateEntry(url, url, RequestFlags(), Http::METHOD_GET); const MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initPeerMcast); HttpRequest *req = HttpRequest::FromUrl(url, mx); + assert(req != nullptr); + StoreEntry *fake = storeCreateEntry(url, url, RequestFlags(), Http::METHOD_GET); psstate = new ps_state(nullptr); psstate->request = req; HTTPMSGLOCK(psstate->request); diff --git a/src/redirect.cc b/src/redirect.cc index c094146ecc..6c2430f8e8 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -108,10 +108,6 @@ redirectHandleReply(void *data, const Helper::Reply &reply) // if we still have anything in other() after all that // parse it into status=, url= and rewrite-url= keys if (replySize) { - /* 2012-06-28: This cast is due to URL::parse() truncating too-long URLs itself. - * At this point altering the helper buffer in that way is not harmful, but annoying. - * When Bug 1961 is resolved and URL::parse has a const API, this needs to die. - */ MemBuf replyBuffer; replyBuffer.init(replySize, replySize); replyBuffer.append(reply.other().content(), reply.other().contentSize()); diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index bbede7bed3..47521c9832 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -339,6 +339,7 @@ Ftp::Server::resetLogin(const char *reason) void Ftp::Server::calcUri(const SBuf *file) { + // TODO: fill a class URL instead of string uri = "ftp://"; uri.append(host); if (port->ftp_track_dirs && master->workingDir.length()) { @@ -723,16 +724,15 @@ Ftp::Server::parseOneRequest() const SBuf *path = (params.length() && CommandHasPathParameter(cmd)) ? ¶ms : NULL; calcUri(path); - char *newUri = xstrdup(uri.c_str()); MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initClient); mx->tcpClient = clientConnection; - HttpRequest *const request = HttpRequest::FromUrl(newUri, mx, method); + HttpRequest *const request = HttpRequest::FromUrl(uri.c_str(), mx, method); if (!request) { debugs(33, 5, "Invalid FTP URL: " << uri); uri.clear(); - safe_free(newUri); return earlyError(EarlyErrorKind::InvalidUri); } + char *newUri = xstrdup(uri.c_str()); request->flags.ftpNative = true; request->http_ver = Http::ProtocolVersion(Ftp::ProtocolVersion().major, Ftp::ProtocolVersion().minor); diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc index 9f808b0086..6b32610eed 100644 --- a/src/servers/Http1Server.cc +++ b/src/servers/Http1Server.cc @@ -132,6 +132,7 @@ Http::One::Server::buildHttpRequest(Http::StreamPointer &context) return false; } + // TODO: move URL parse into Http Parser and INVALID_URL into the above parse error handling MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initClient); mx->tcpClient = clientConnection; if ((request = HttpRequest::FromUrl(http->uri, mx, parser_->method())) == NULL) { diff --git a/src/store_digest.cc b/src/store_digest.cc index 549b8a241e..3509ca2993 100644 --- a/src/store_digest.cc +++ b/src/store_digest.cc @@ -401,10 +401,6 @@ storeDigestRebuildStep(void *datanotused) static void storeDigestRewriteStart(void *datanotused) { - RequestFlags flags; - char *url; - StoreEntry *e; - assert(store_digest); /* prevent overlapping if rewrite schedule is too tight */ @@ -414,17 +410,21 @@ storeDigestRewriteStart(void *datanotused) } debugs(71, 2, "storeDigestRewrite: start rewrite #" << sd_state.rewrite_count + 1); - /* make new store entry */ - url = internalLocalUri("/squid-internal-periodic/", SBuf(StoreDigestFileName)); + + const char *url = internalLocalUri("/squid-internal-periodic/", SBuf(StoreDigestFileName)); + const MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initCacheDigest); + auto req = HttpRequest::FromUrl(url, mx); + + RequestFlags flags; flags.cachable = true; - e = storeCreateEntry(url, url, flags, Http::METHOD_GET); + + StoreEntry *e = storeCreateEntry(url, url, flags, Http::METHOD_GET); assert(e); sd_state.rewrite_lock = e; debugs(71, 3, "storeDigestRewrite: url: " << url << " key: " << e->getMD5Text()); - const MasterXaction::Pointer mx = new MasterXaction(XactionInitiator::initCacheDigest); - e->mem_obj->request = HttpRequest::FromUrl(url, mx); - /* wait for rebuild (if any) to finish */ + e->mem_obj->request = req; + /* wait for rebuild (if any) to finish */ if (sd_state.rebuild_lock) { debugs(71, 2, "storeDigestRewriteStart: waiting for rebuild to finish."); return; diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc index 55de070fd3..66c267b9a8 100644 --- a/src/tests/stub_HttpRequest.cc +++ b/src/tests/stub_HttpRequest.cc @@ -47,7 +47,7 @@ int HttpRequest::prefixLen() const STUB_RETVAL(0) void HttpRequest::swapOut(StoreEntry *) STUB void HttpRequest::pack(Packable *) const STUB void HttpRequest::httpRequestPack(void *, Packable *) STUB -HttpRequest * HttpRequest::FromUrl(char *, const MasterXaction::Pointer &, const HttpRequestMethod &) STUB_RETVAL(NULL) +HttpRequest * HttpRequest::FromUrl(const char *, const MasterXaction::Pointer &, const HttpRequestMethod &) STUB_RETVAL(NULL) ConnStateData *HttpRequest::pinnedConnection() STUB_RETVAL(NULL) const SBuf HttpRequest::storeId() STUB_RETVAL(SBuf(".")) void HttpRequest::ignoreRange(const char *) STUB diff --git a/src/tests/stub_url.cc b/src/tests/stub_url.cc index 2a7a6be718..426a300a38 100644 --- a/src/tests/stub_url.cc +++ b/src/tests/stub_url.cc @@ -14,7 +14,7 @@ #include "URL.h" URL::URL(AnyP::UriScheme const &) {STUB} void URL::touch() STUB -bool URL::parse(const HttpRequestMethod&, char *) STUB_RETVAL(true) +bool URL::parse(const HttpRequestMethod&, const char *) STUB_RETVAL(true) void URL::host(const char *) STUB static SBuf nil; const SBuf &URL::path() const STUB_RETVAL(nil) diff --git a/src/url.cc b/src/url.cc index 35bbcc69fb..1d70201e44 100644 --- a/src/url.cc +++ b/src/url.cc @@ -188,7 +188,7 @@ urlParseProtocol(const char *b) * being "end of host with implied path of /". */ bool -URL::parse(const HttpRequestMethod& method, char *url) +URL::parse(const HttpRequestMethod& method, const char *url) { LOCAL_ARRAY(char, proto, MAX_URL); LOCAL_ARRAY(char, login, MAX_URL); @@ -205,8 +205,6 @@ URL::parse(const HttpRequestMethod& method, char *url) proto[0] = foundHost[0] = urlpath[0] = login[0] = '\0'; if ((l = strlen(url)) + Config.appendDomainLen > (MAX_URL - 1)) { - /* terminate so it doesn't overflow other buffers */ - *(url + (MAX_URL >> 1)) = '\0'; debugs(23, DBG_IMPORTANT, MYNAME << "URL too large (" << l << " bytes)"); return false; } diff --git a/src/urn.cc b/src/urn.cc index 71605325d4..f27b68224a 100644 --- a/src/urn.cc +++ b/src/urn.cc @@ -145,23 +145,23 @@ UrnState::setUriResFromRequest(HttpRequest *r) } SBuf uri = r->url.path(); + // TODO: use class URL instead of generating a string and re-parsing LOCAL_ARRAY(char, local_urlres, 4096); char *host = getHost(uri); snprintf(local_urlres, 4096, "http://%s/uri-res/N2L?urn:" SQUIDSBUFPH, host, SQUIDSBUFPRINT(uri)); safe_free(host); safe_free(urlres); - urlres = xstrdup(local_urlres); - urlres_r = HttpRequest::FromUrl(urlres, r->masterXaction); + urlres_r = HttpRequest::FromUrl(local_urlres, r->masterXaction); - if (urlres_r == NULL) { - debugs(52, 3, "urnStart: Bad uri-res URL " << urlres); + if (!urlres_r) { + debugs(52, 3, "Bad uri-res URL " << local_urlres); ErrorState *err = new ErrorState(ERR_URN_RESOLVE, Http::scNotFound, r); - err->url = urlres; - urlres = NULL; + err->url = xstrdup(local_urlres); errorAppendEntry(entry, err); return; } + urlres = xstrdup(local_urlres); urlres_r->header.putStr(Http::HdrType::ACCEPT, "text/plain"); } @@ -395,7 +395,6 @@ urnParseReply(const char *inbuf, const HttpRequestMethod& m) { char *buf = xstrdup(inbuf); char *token; - char *url; char *host; url_entry *list; url_entry *old; @@ -415,8 +414,7 @@ urnParseReply(const char *inbuf, const HttpRequestMethod& m) safe_free(old); } - url = xstrdup(token); - host = urlHostname(url); + host = urlHostname(token); if (NULL == host) continue; @@ -432,11 +430,11 @@ urnParseReply(const char *inbuf, const HttpRequestMethod& m) list[i].rtt = 0; #endif - list[i].url = url; + list[i].url = xstrdup(token); list[i].host = xstrdup(host); // TODO: Use storeHas() or lock/unlock entry to avoid creating unlocked // ones. - list[i].flags.cached = storeGetPublic(url, m) ? 1 : 0; + list[i].flags.cached = storeGetPublic(list[i].url, m) ? 1 : 0; ++i; } -- 2.47.2