From: Amos Jeffries Date: Sun, 9 Jul 2017 08:16:33 +0000 (+1200) Subject: Bug 1961 extra: Convert the URL::parse method API to take const URI strings X-Git-Tag: SQUID_4_0_22~22 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=47d5b9d8da24d997c99be7edf63d62c83a88f807;p=thirdparty%2Fsquid.git 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. --- 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 cb29a9698c..91b16d1a4a 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 562851063c..443b81e3f3 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -201,7 +201,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 33afe51dc7..86e118a61c 100644 --- a/src/adaptation/ecap/MessageRep.cc +++ b/src/adaptation/ecap/MessageRep.cc @@ -201,10 +201,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 213e849217..91e40b47d8 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 c6cdb1adbb..1e3bd51f63 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -1371,19 +1371,20 @@ peerCountMcastPeersStart(void *data) { 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; 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 d92a48af81..91c02fb7ef 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 be4530109f..a497f6ee71 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,19 +410,22 @@ 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); - HttpRequest *req = HttpRequest::FromUrl(url, mx); e->mem_obj->request = req; HTTPMSGLOCK(e->mem_obj->request); - /* wait for rebuild (if any) to finish */ + /* 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 59ecb5ca3b..363509e90f 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/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; }