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 e153861d97589014791ef555347c8c8bd87586c8..ccfe998e8b097ca4da66fca6d1c03e073f66705b 100644 (file)
--- 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 4366f66be17aa283ae79ec62e8c94ec0faecc6e5..509d2a44dfba6be15386e8ac2898e8fb9be18f61 100644 (file)
--- 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<HttpRequest> req(new HttpRequest(mx));
     if (req->url.parse(method, url)) {

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 0d8b2ca8f241bc9545d4028ff7cc6a2c957665ea..dd22412449f671c5f567007777e331e529774fa6 100644 (file)
--- 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 88e856698c5a3d7974227b78bb6fd8c59ede2cd4..ccf8d62c4599c82b11ac76aaac722e74c21b1fdd 100644 (file)
--- 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 c152f29bf3d2f4974476745b432c77aec14bf1d4..c2e23bbe27009b9027c2031a6c7ca4ec9876c397 100644 (file)
--- 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 31e37ba3cb05afc7000a5ae399a4834e2f4a77fb..ac02172e8f02ddeb46d4c4a1d9a474d3243d302a 100644 (file)
--- 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 664ddf0e1d9bac005b35982f7a78c407b588e458..6d18f02a15324d21e5be199ecb5f532432c65fbf 100644 (file)
--- 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<char*>(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 fc662d8bf58b5e44339194f52529fbdce849c002..92b134ae26d1d2c89a73f54c76fbb4f77d2e2e6d 100644 (file)
--- a/src/mgr/Inquirer.cc
+++ b/src/mgr/Inquirer.cc
@@ -74,8 +74,7 @@ Mgr::Inquirer::start()
 
     std::unique_ptr<MemBuf> 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 65ae2ef59077b934702b38a395cc174665a8bebd..afb245ade35d9e07907b5481fea82aaf1eaf0e13 100644 (file)
--- 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 c094146eccb0b812e9b725b6d19b2f56fd898303..6c2430f8e8d1712ac13b6b699dfb5348540d97bb 100644 (file)
--- 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 bbede7bed3727f241e7dc1f78349329b677648a6..47521c983280deb9ee3024d3205e32b637f1f0f7 100644 (file)
--- 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)) ?
                        &params : 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 9f808b0086d7aa6764e63b017db2211db876f7cf..6b32610eedddeff3e538c5ef3d814f33578c1d10 100644 (file)
--- 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 549b8a241ecf30518f3b167e9ad032a219857a35..3509ca2993048b86eb8328e1aad2b21abe746974 100644 (file)
--- 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 55de070fd36420876f81112a8e45b073f189b678..66c267b9a88878a20c80b5982e4a06b160fa5cd5 100644 (file)
--- 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 2a7a6be71894b1746d233a29a7be0149cff8f9d5..426a300a38aa2494ab715d0f092d0275f7937743 100644 (file)
--- 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 35bbcc69fb39c388f3d098266cfcaa7cb33ccdd2..1d70201e44730a19fb22a290ead341014075bcad 100644 (file)
--- 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 71605325d455bafabe99ceacbab3a0b4a84cbca5..f27b68224a2d87d244abcd9c2edda546b527d3c2 100644 (file)
--- 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;
     }