Bug 1961 partial: Redesign urlParse API

The urlParse modified to require an HttpRequest object as parameter and return
boolean value to indicate URL parsing status.

This is a Measurement Factory project

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index b786eec97691af2196e96be032807616a7f1b797..0b68e29e945ec160fbe52c80b5fba5d1b08fedd0 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -327,14 +327,11 @@ HttpRequest::parseFirstLine(const char *start, const char *end)
 
     * (char *) end = '\0';     // temp terminate URI, XXX dangerous?
 
-    HttpRequest *tmp = urlParse(method, (char *) start, this);
+    const bool ret = urlParse(method, (char *) start, *this);
 
     * (char *) end = save;
 
-    if (NULL == tmp)
-        return false;
-
-    return true;
+    return ret;
 }
 
 /* swaps out request using httpRequestPack */
@@ -520,7 +517,10 @@ HttpRequest::expectingBody(const HttpRequestMethod &, int64_t &theSize) const
 HttpRequest *
 HttpRequest::CreateFromUrl(char * url, const HttpRequestMethod& method)
 {
-    return urlParse(method, url, NULL);
+    std::unique_ptr<HttpRequest> req(new HttpRequest());
+    if (urlParse(method, url, *req))
+        return req.release();
+    return nullptr;
 }
 
 /**

diff --git a/src/URL.h b/src/URL.h
index 4bbe45381c74797f94a6f9f340661278d4b5924a..6d2eeaba4ac384509f10d41c4dacaf054e6bb99e 100644 (file)
--- a/src/URL.h
+++ b/src/URL.h
@@ -166,7 +166,7 @@ class HttpRequest;
 class HttpRequestMethod;
 
 void urlInitialize(void);
-HttpRequest *urlParse(const HttpRequestMethod&, char *, HttpRequest *request = NULL);
+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/adaptation/ecap/MessageRep.cc b/src/adaptation/ecap/MessageRep.cc
index 99a1c151dcd088f6137ccddfeb34cb6230a4a22b..83e5165e274651da3c4367b554df8a6c466d77d5 100644 (file)
--- a/src/adaptation/ecap/MessageRep.cc
+++ b/src/adaptation/ecap/MessageRep.cc
@@ -203,7 +203,7 @@ Adaptation::Ecap::RequestLineRep::uri(const Area &aUri)
     // Can we change urlParse API to remove the method parameter?
     // TODO: optimize: urlPath should take constant URL buffer
     char *buf = xstrdup(aUri.toString().c_str());
-    const bool ok = urlParse(theMessage.method, buf, &theMessage);
+    const bool ok = urlParse(theMessage.method, buf, theMessage);
     xfree(buf);
     Must(ok);
 }

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 782002c286345ca6a6a92b8eda92f773d293a8b4..450650275017ef813b8cb86036cf4994a6b8f0dd 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1276,7 +1276,7 @@ ClientRequestContext::clientRedirectDone(const Helper::Reply &reply)
                 // XXX: validate the URL properly *without* generating a whole new request object right here.
                 // XXX: the clone() should be done only AFTER we know the new URL is valid.
                 HttpRequest *new_request = old_request->clone();
-                if (urlParse(old_request->method, const_cast<char*>(urlNote), new_request)) {
+                if (urlParse(old_request->method, const_cast<char*>(urlNote), *new_request)) {
                     debugs(61, 2, "URL-rewriter diverts URL from " << old_request->effectiveRequestUri() << " to " << new_request->effectiveRequestUri());
 
                     // update the new request to flag the re-writing was done on it

diff --git a/src/url.cc b/src/url.cc
index 08e1e1bd320badfb16ed36a3a3b61f9d07a31f01..b3ae1806c23a5ef24efe8f0bfd307fdbe157690d 100644 (file)
--- a/src/url.cc
+++ b/src/url.cc
@@ -16,15 +16,15 @@
 #include "SquidString.h"
 #include "URL.h"
 
-static HttpRequest *urlParseFinish(const HttpRequestMethod& method,
+static bool urlParseFinish(const HttpRequestMethod& method,
                                    const AnyP::ProtocolType protocol,
                                    const char *const protoStr,
                                    const char *const urlpath,
                                    const char *const host,
                                    const SBuf &login,
                                    const int port,
-                                   HttpRequest *request);
-static HttpRequest *urnParse(const HttpRequestMethod& method, char *urn, HttpRequest *request);
+                                   HttpRequest &request);
+static bool urnParse(const HttpRequestMethod& method, char *urn, HttpRequest &request);
 static const char valid_hostname_chars_u[] =
     "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
     "abcdefghijklmnopqrstuvwxyz"
@@ -179,8 +179,7 @@ urlParseProtocol(const char *b)
 /*
  * Parse a URI/URL.
  *
- * If the 'request' arg is non-NULL, put parsed values there instead
- * of allocating a new HttpRequest.
+ * Stores parsed values in the `request` argument.
  *
  * This abuses HttpRequest as a way of representing the parsed url
  * and its components.
@@ -197,8 +196,8 @@ urlParseProtocol(const char *b)
  * its partial or not (ie, it handles the case of no trailing slash as
  * being "end of host with implied path of /".
  */
-HttpRequest *
-urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
+bool
+urlParse(const HttpRequestMethod& method, char *url, HttpRequest &request)
 {
     LOCAL_ARRAY(char, proto, MAX_URL);
     LOCAL_ARRAY(char, login, MAX_URL);
@@ -218,14 +217,14 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
         /* terminate so it doesn't overflow other buffers */
         *(url + (MAX_URL >> 1)) = '\0';
         debugs(23, DBG_IMPORTANT, "urlParse: URL too large (" << l << " bytes)");
-        return NULL;
+        return false;
     }
     if (method == Http::METHOD_CONNECT) {
         port = CONNECT_PORT;
 
         if (sscanf(url, "[%[^]]]:%d", host, &port) < 1)
             if (sscanf(url, "%[^:]:%d", host, &port) < 1)
-                return NULL;
+                return false;
 
     } else if ((method == Http::METHOD_OPTIONS || method == Http::METHOD_TRACE) &&
                URL::Asterisk().cmp(url) == 0) {
@@ -243,12 +242,12 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
             *dst = *src;
         }
         if (i >= l)
-            return NULL;
+            return false;
         *dst = '\0';
 
         /* Then its :// */
         if ((i+3) > l || *src != ':' || *(src + 1) != '/' || *(src + 2) != '/')
-            return NULL;
+            return false;
         i += 3;
         src += 3;
 
@@ -266,7 +265,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
          * been -given- a valid URL and the path is just '/'.
          */
         if (i > l)
-            return NULL;
+            return false;
         *dst = '\0';
 
         // bug 3074: received 'path' starting with '?', '#', or '\0' implies '/'
@@ -283,7 +282,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
 
         /* We -could- be at the end of the buffer here */
         if (i > l)
-            return NULL;
+            return false;
         /* If the URL path is empty we set it to be "/" */
         if (dst == urlpath) {
             *dst = '/';
@@ -342,7 +341,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
         // Bug 3183 sanity check: If scheme is present, host must be too.
         if (protocol != AnyP::PROTO_NONE && host[0] == '\0') {
             debugs(23, DBG_IMPORTANT, "SECURITY ALERT: Missing hostname in URL '" << url << "'. see access.log for details.");
-            return NULL;
+            return false;
         }
 
         if (t && *t == ':') {
@@ -373,7 +372,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
 
     if (Config.onoff.check_hostnames && strspn(host, Config.onoff.allow_underscore ? valid_hostname_chars_u : valid_hostname_chars) != strlen(host)) {
         debugs(23, DBG_IMPORTANT, "urlParse: Illegal character in hostname '" << host << "'");
-        return NULL;
+        return false;
     }
 
     /* For IPV6 addresses also check for a colon */
@@ -387,12 +386,12 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
     /* reject duplicate or leading dots */
     if (strstr(host, "..") || *host == '.') {
         debugs(23, DBG_IMPORTANT, "urlParse: Illegal hostname '" << host << "'");
-        return NULL;
+        return false;
     }
 
     if (port < 1 || port > 65535) {
         debugs(23, 3, "urlParse: Invalid port '" << port << "'");
-        return NULL;
+        return false;
     }
 
 #if HARDCODE_DENY_PORTS
@@ -400,7 +399,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
      * maybe someone wants them hardcoded... */
     if (port == 7 || port == 9 || port == 19) {
         debugs(23, DBG_CRITICAL, "urlParse: Deny access to port " << port);
-        return NULL;
+        return false;
     }
 #endif
 
@@ -410,7 +409,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
         switch (Config.uri_whitespace) {
 
         case URI_WHITESPACE_DENY:
-            return NULL;
+            return false;
 
         case URI_WHITESPACE_ALLOW:
             break;
@@ -446,7 +445,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
  * non-NULL, put parsed values there instead of allocating a new
  * HttpRequest.
  */
-static HttpRequest *
+static bool
 urlParseFinish(const HttpRequestMethod& method,
                const AnyP::ProtocolType protocol,
                const char *const protoStr, // for unknown protocols
@@ -454,30 +453,21 @@ urlParseFinish(const HttpRequestMethod& method,
                const char *const host,
                const SBuf &login,
                const int port,
-               HttpRequest *request)
+               HttpRequest &request)
 {
-    if (NULL == request)
-        request = new HttpRequest(method, protocol, protoStr, urlpath);
-    else {
-        request->initHTTP(method, protocol, protoStr, urlpath);
-    }
-
-    request->url.host(host);
-    request->url.userInfo(login);
-    request->url.port(port);
-    return request;
+    request.initHTTP(method, protocol, protoStr, urlpath);
+    request.url.host(host);
+    request.url.userInfo(login);
+    request.url.port(port);
+    return true;
 }
 
-static HttpRequest *
-urnParse(const HttpRequestMethod& method, char *urn, HttpRequest *request)
+static bool
+urnParse(const HttpRequestMethod& method, char *urn, HttpRequest &request)
 {
     debugs(50, 5, "urnParse: " << urn);
-    if (request) {
-        request->initHTTP(method, AnyP::PROTO_URN, "urn", urn + 4);
-        return request;
-    }
-
-    return new HttpRequest(method, AnyP::PROTO_URN, "urn", urn + 4);
+    request.initHTTP(method, AnyP::PROTO_URN, "urn", urn + 4);
+    return true;
 }
 
 void