Audit updates

* rename HttpRequestMethod(char*) to HttpRequestMethodXXX() in order to
  assist removal since it is deprecated now
 - plus code polishing and unit-test updates to work with this as a
   method instead of constructor

* fix several potential out-of-bounds SBuf and MemBlob accesses

* reduce performance regression parsing ICAP/eCAP traffic

* simplify and optimize clear() operation

* documentation clarifications

* typos and spelling fixes

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index acc944975db1d661a8699df3cb06abfc59689c91..56628f120bebb271f25beef238fe4209586df0aa 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -280,8 +280,10 @@ HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::Statu
         return false;
     }
 
-    /* See if the request buffer starts with a known HTTP request method. */
-    if (HttpRequestMethod(buf->content()) == Http::METHOD_NONE) {
+    /* See if the request buffer starts with a non-whitespace HTTP request 'method'. */
+    HttpRequestMethod m;
+    m.HttpRequestMethodXXX(buf->content());
+    if (m == Http::METHOD_NONE) {
         debugs(73, 3, "HttpRequest::sanityCheckStartLine: did not find HTTP request method");
         *error = Http::scInvalidHeader;
         return false;
@@ -293,14 +295,16 @@ HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::Statu
 bool
 HttpRequest::parseFirstLine(const char *start, const char *end)
 {
-    const char *t = start + strcspn(start, w_space);
-    SBuf m(start, start-t); // XXX: SBuf ctor allocates and data-copies. performance regression.
-    method = HttpRequestMethod(m);
+    method.HttpRequestMethodXXX(start);
 
     if (method == Http::METHOD_NONE)
         return false;
 
-    start = t + strspn(t, w_space); // XXX: breaks if whitespace exists in URL
+    // XXX: performance regression, strcspn() over the method bytes a second time.
+    // cheaper than allocate+copy+deallocate cycle to SBuf convert a piece of start.
+    const char *t = start + strcspn(start, w_space);
+
+    start = t + strspn(t, w_space); // skip w_space after method
 
     const char *ver = findTrailingHTTPVersion(start, end);
 

diff --git a/src/acl/MethodData.cc b/src/acl/MethodData.cc
index 295bd0639a9fa06d025073816d434f5154d1cdf5..16cd9c8757fe97fa86ae7a305fe2fce135eaa0a8 100644 (file)
--- a/src/acl/MethodData.cc
+++ b/src/acl/MethodData.cc
@@ -65,7 +65,9 @@ ACLMethodData::parse()
 
     for (Tail = &values; *Tail; Tail = &((*Tail)->next));
     while ((t = strtokFile())) {
-        CbDataList<HttpRequestMethod> *q = new CbDataList<HttpRequestMethod> (HttpRequestMethod(t));
+        HttpRequestMethod m;
+        m.HttpRequestMethodXXX(t);
+        CbDataList<HttpRequestMethod> *q = new CbDataList<HttpRequestMethod>(m);
         if (q->element == Http::METHOD_PURGE)
             ++ThePurgeCount; // configuration code wants to know
         *(Tail) = q;

diff --git a/src/client_side.cc b/src/client_side.cc
index 1f0625d7bc2b9abcd2c0dc82dc94f3db703bc5ea..82b17ea58fdecedb64653984ac61e7dd394a3311 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2014,7 +2014,7 @@ prepareAcceleratedURL(ConnStateData * conn, ClientHttpRequest *http, const Http1
     SBuf url = hp->requestUri(); // use full provided URI if we abort
     do { // use a loop so we can break out of it
         ::Parser::Tokenizer tok(url);
-        if (tok.remaining()[0] == '/')
+        if (tok.skip('/')) // origin-form URL already.
             break;
 
         if (conn->port->vhost)
@@ -2048,7 +2048,7 @@ prepareAcceleratedURL(ConnStateData * conn, ClientHttpRequest *http, const Http1
 
 #if SHOULD_REJECT_UNKNOWN_URLS
     // reject URI which are not well-formed even after the processing above
-    if (url[0] != '/') {
+    if (url.isEmpty() || url[0] != '/') {
         hp->request_parse_status = Http::scBadRequest;
         return conn->abortRequestParsing("error:invalid-request");
     }
@@ -2110,8 +2110,6 @@ prepareAcceleratedURL(ConnStateData * conn, ClientHttpRequest *http, const Http1
 static void
 prepareTransparentURL(ConnStateData * conn, ClientHttpRequest *http, const Http1::RequestParserPointer &hp)
 {
-    static char ipbuf[MAX_IPSTRLEN];
-
     // TODO Must() on URI !empty when the parser supports throw. For now avoid assert().
     if (!hp->requestUri().isEmpty() && hp->requestUri()[0] != '/')
         return; /* already in good shape */
@@ -2129,6 +2127,7 @@ prepareTransparentURL(ConnStateData * conn, ClientHttpRequest *http, const Http1
         /* Put the local socket IP address as the hostname.  */
         const int url_sz = hp->requestUri().length() + 32 + Config.appendDomainLen;
         http->uri = (char *)xcalloc(url_sz, 1);
+        static char ipbuf[MAX_IPSTRLEN];
         http->getConn()->clientConnection->local.toHostStr(ipbuf,MAX_IPSTRLEN);
         snprintf(http->uri, url_sz, "%s://%s:%d" SQUIDSBUFPH,
                  AnyP::UriScheme(http->getConn()->port->transport.protocol).c_str(),
@@ -2220,7 +2219,8 @@ parseHttpRequest(ConnStateData *csd, const Http1::RequestParserPointer &hp)
     /* set url */
     // XXX: c_str() does re-allocate but here replaces explicit malloc/free.
     // when internalCheck() accepts SBuf removing this will be a net gain for performance.
-    const char *url = SBuf(hp->requestUri()).c_str();
+    SBuf tmp(hp->requestUri());
+    const char *url = tmp.c_str();
 
     debugs(33,5, HERE << "repare absolute URL from " <<
            (csd->transparent()?"intercept":(csd->port->flags.accelSurrogate ? "accel":"")));
@@ -2477,8 +2477,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
     bool expectBody = false;
 
     // temporary hack to avoid splitting this huge function with sensitive code
-    const bool isFtp = (hp == NULL);
-    const HttpRequestMethod &method = !isFtp ? hp->method() : Http::METHOD_NONE; // XXX: or should this be GET ?
+    const bool isFtp = !hp;
     if (isFtp) {
         // In FTP, case, we already have the request parsed and checked, so we
         // only need to go through the final body/conn setup to doCallouts().
@@ -2547,7 +2546,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
             setLogUri(http, http->uri,  true);
             clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
             assert (repContext);
-            repContext->setReplyToError(ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, method, http->uri,
+            repContext->setReplyToError(ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, hp->method(), http->uri,
                                         conn->clientConnection->remote, NULL, NULL, NULL);
             assert(context->http->out.offset == 0);
             context->pullData();
@@ -2661,7 +2660,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp,
         unsupportedTe = te.size() && te != "identity";
     } // else implied identity coding
 
-    mustReplyToOptions = (method == Http::METHOD_OPTIONS) &&
+    mustReplyToOptions = (request->method == Http::METHOD_OPTIONS) &&
                          (request->header.getInt64(HDR_MAX_FORWARDS) == 0);
     if (!urlCheckRequest(request.getRaw()) || mustReplyToOptions || unsupportedTe) {
         clientStreamNode *node = context->getClientReplyContext();
@@ -3094,7 +3093,6 @@ ConnStateData::clientParseRequests()
         if (needProxyProtocolHeader_ && !parseProxyProtocolHeader())
             break;
 
-        Http::ProtocolVersion http_ver;
         if (ClientSocketContext *context = parseOneRequest()) {
             debugs(33, 5, clientConnection << ": done parsing a request");
 

diff --git a/src/htcp.cc b/src/htcp.cc
index e430e67cd27345985313b4531a0809346d9d43d3..3e356eee2a02dbb2bc8cfcac06d0c7f212ae3418 100644 (file)
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -722,10 +722,8 @@ htcpUnpackSpecifier(char *buf, int sz)
      */
     *buf = '\0';
 
-    /*
-     * Parse the request
-     */
-    method = HttpRequestMethod(s->method);
+    // Parse the request
+    method.HttpRequestMethodXXX(s->method);
 
     s->request = HttpRequest::CreateFromUrlAndMethod(s->uri, method == Http::METHOD_NONE ? HttpRequestMethod(Http::METHOD_GET) : method);
 

diff --git a/src/http/RequestMethod.cc b/src/http/RequestMethod.cc
index 02703a8e7aed71846a50281e32b9d43149f1da01..0c6b0b28df64786adcb3f650bd49246274ac602c 100644 (file)
--- a/src/http/RequestMethod.cc
+++ b/src/http/RequestMethod.cc
@@ -27,8 +27,15 @@ operator++ (Http::MethodType &aMethod)
  *
  * \deprecated use SBuf constructor instead
  */
-HttpRequestMethod::HttpRequestMethod(char const *begin) : theMethod(Http::METHOD_NONE)
+void
+HttpRequestMethod::HttpRequestMethodXXX(char const *begin)
 {
+    // XXX: performance regression due to this method no longer being a constructor
+    // ensure the members are empty/default values before any of the early-return
+    // optimizations can be used.
+    theMethod = Http::METHOD_NONE;
+    theImage.clear();
+
     if (begin == NULL)
         return;
 
@@ -59,17 +66,15 @@ HttpRequestMethod::HttpRequestMethod(char const *begin) : theMethod(Http::METHOD
 
 /**
  * Construct a HttpRequestMethod from an SBuf string such as "GET"
- * or from a range of chars such as "GET" from "GETFOOBARBAZ"
+ * or from a range of chars such as "FOO" from buffer "GETFOOBARBAZ"
  *
- * Assumes the s parameter contains only the method string
+ * Assumes the s parameter contains only the characters representing the method name
  */
 HttpRequestMethod::HttpRequestMethod(const SBuf &s) : theMethod(Http::METHOD_NONE)
 {
     if (s.isEmpty())
         return;
 
-    // XXX: still check for missing method name?
-
     // TODO: Optimize this linear search.
     for (++theMethod; theMethod < Http::METHOD_ENUM_END; ++theMethod) {
         // RFC 2616 section 5.1.1 - Method names are case-sensitive

diff --git a/src/http/RequestMethod.h b/src/http/RequestMethod.h
index 826413510bf505fac800ce39616c38005a10dba6..a95861fc522febd27be5d673a01b6f0fad8dff62 100644 (file)
--- a/src/http/RequestMethod.h
+++ b/src/http/RequestMethod.h
@@ -28,9 +28,10 @@ class HttpRequestMethod : public RefCountable
 public:
     HttpRequestMethod() : theMethod(Http::METHOD_NONE), theImage() {}
     HttpRequestMethod(Http::MethodType const aMethod) : theMethod(aMethod), theImage() {}
-    explicit HttpRequestMethod(char const *);
     explicit HttpRequestMethod(const SBuf &);
 
+    void HttpRequestMethodXXX(char const *); // deprecated old c-string to SBuf converter.
+
     HttpRequestMethod & operator = (const HttpRequestMethod& aMethod) {
         theMethod = aMethod.theMethod;
         theImage = aMethod.theImage;

diff --git a/src/http/one/Parser.h b/src/http/one/Parser.h
index 63c9318c18ed69c8d2166452a702a984621cf610..2c9c584b4e81ecd9692a219491f0b0c09a49e2f7 100644 (file)
--- a/src/http/one/Parser.h
+++ b/src/http/one/Parser.h
@@ -38,10 +38,11 @@ public:
     typedef SBuf::size_type size_type;
 
     Parser() : parsingStage_(HTTP_PARSE_NONE) {}
+    virtual ~Parser() {}
 
     /// Set this parser back to a default state.
     /// Will DROP any reference to a buffer (does not free).
-    virtual void clear();
+    virtual void clear() = 0;
 
     /// attempt to parse a message from the buffer
     /// \retval true if a full message was found and parsed

diff --git a/src/http/one/RequestParser.cc b/src/http/one/RequestParser.cc
index 3226be5c757a27dac98b0229457a2b437b8c94e4..64cd6616c5a01ccedebac7613df5b7c84ae257dc 100644 (file)
--- a/src/http/one/RequestParser.cc
+++ b/src/http/one/RequestParser.cc
@@ -6,17 +6,14 @@
 #include "profiler/Profiler.h"
 #include "SquidConfig.h"
 
-void
-Http::One::RequestParser::clear()
+Http::One::RequestParser::RequestParser() :
+        Parser(),
+        request_parse_status(Http::scNone)
 {
-    Http1::Parser::clear();
-
-    request_parse_status = Http::scNone;
     req.start = req.end = -1;
     req.m_start = req.m_end = -1;
     req.u_start = req.u_end = -1;
     req.v_start = req.v_end = -1;
-    method_ = HttpRequestMethod();
 }
 
 /**

diff --git a/src/http/one/RequestParser.h b/src/http/one/RequestParser.h
index d4693b391eb1e01924d4716d0014d825f936db1f..c72b54dc89107b06b0e5619dddb22fe8b9537863 100644 (file)
--- a/src/http/one/RequestParser.h
+++ b/src/http/one/RequestParser.h
@@ -19,9 +19,11 @@ namespace One {
 class RequestParser : public Http1::Parser
 {
 public:
+    RequestParser();
+    virtual ~RequestParser() {}
+
     /* Http::One::Parser API */
-    RequestParser() : Parser() {clear();}
-    virtual void clear();
+    virtual void clear() {*this = RequestParser();}
     virtual Http1::Parser::size_type firstLineSize() const {return req.end - req.start + 1;}
     virtual bool parse(const SBuf &aBuf);
 
@@ -53,7 +55,7 @@ private:
     /// what request method has been found on the first line
     HttpRequestMethod method_;
 
-    /// raw copy of the origina client reqeust-line URI field
+    /// raw copy of the original client reqeust-line URI field
     SBuf uri_;
 };
 

diff --git a/src/mgr/ActionParams.cc b/src/mgr/ActionParams.cc
index 927f5ff6dde93413bc6eddfe9bbf786fa907d2c8..6865b30b65cadd7440dd3cee774887ddc6c10d0b 100644 (file)
--- a/src/mgr/ActionParams.cc
+++ b/src/mgr/ActionParams.cc
@@ -23,7 +23,7 @@ Mgr::ActionParams::ActionParams(const Ipc::TypedMsgHdr &msg)
 
     String method;
     msg.getString(method);
-    httpMethod = HttpRequestMethod(method.termedBuf());
+    httpMethod.HttpRequestMethodXXX(method.termedBuf());
 
     msg.getPod(httpFlags);
     msg.getString(httpOrigin);

diff --git a/src/tests/testHttp1Parser.cc b/src/tests/testHttp1Parser.cc
index 742bab8039a37c27a42c017e2e02e8e013a39c4d..7bfff5ec033aca04eabf245741e622a7ee2bb572 100644 (file)
--- a/src/tests/testHttp1Parser.cc
+++ b/src/tests/testHttp1Parser.cc
@@ -734,7 +734,7 @@ testHttp1Parser::testParseRequestLineMethods()
             .suffixSz = 0,
             .methodStart = 0,
             .methodEnd = 0,
-            .method = HttpRequestMethod("."),
+            .method = HttpRequestMethod(SBuf(".")),
             .uriStart = 2,
             .uriEnd = 2,
             .uri = "/",
@@ -786,7 +786,7 @@ testHttp1Parser::testParseRequestLineMethods()
             .suffixSz = 0,
             .methodStart = 0,
             .methodEnd = 9,
-            .method = HttpRequestMethod("HELLOWORLD"),
+            .method = HttpRequestMethod(SBuf("HELLOWORLD")),
             .uriStart = 11,
             .uriEnd = 11,
             .uri = "/",
@@ -1002,7 +1002,7 @@ testHttp1Parser::testParseRequestLineInvalid()
             .suffixSz = 0,
             .methodStart = 0,
             .methodEnd = 0,
-            .method = HttpRequestMethod("/"),
+            .method = HttpRequestMethod(SBuf("/")),
             .uriStart = 2,
             .uriEnd = 9,
             .uri = "HTTP/1.0",
@@ -1031,7 +1031,7 @@ testHttp1Parser::testParseRequestLineInvalid()
             .suffixSz = 0,
             .methodStart = 0,
             .methodEnd = 0,
-            .method = HttpRequestMethod("/"),
+            .method = HttpRequestMethod(SBuf("/")),
             .uriStart = 2,
             .uriEnd = 9,
             .uri = "HTTP/1.0",
@@ -1122,7 +1122,7 @@ testHttp1Parser::testParseRequestLineInvalid()
             .suffixSz = 0,
             .methodStart = 0,
             .methodEnd = 3,
-            .method = HttpRequestMethod(SBuf("GET\x0B")),
+            .method = HttpRequestMethod(SBuf("GET\x0B", 4)),
             .uriStart = 5,
             .uriEnd = 5,
             .uri = "/",

diff --git a/src/tests/testHttpRequestMethod.cc b/src/tests/testHttpRequestMethod.cc
index a541f394217cb806f03dcff5155a2ba18880b321..4b94d2606f128727776e86455419735b5d6150fd 100644 (file)
--- a/src/tests/testHttpRequestMethod.cc
+++ b/src/tests/testHttpRequestMethod.cc
@@ -24,24 +24,49 @@ CPPUNIT_TEST_SUITE_REGISTRATION( testHttpRequestMethod );
 void
 testHttpRequestMethod::testConstructCharStart()
 {
+    // string in SBuf
+
+    /* parse an empty string -> Http::METHOD_NONE */
+    CPPUNIT_ASSERT(HttpRequestMethod(SBuf()) == Http::METHOD_NONE);
+
+    /* parsing a literal should work */
+    CPPUNIT_ASSERT(HttpRequestMethod(SBuf("GET")) == Http::METHOD_GET);
+    CPPUNIT_ASSERT(HttpRequestMethod(SBuf("QWERTY")) == Http::METHOD_OTHER);
+
+    // string in char*
+
     /* parse an empty string -> Http::METHOD_NONE */
-    CPPUNIT_ASSERT(HttpRequestMethod(NULL) == Http::METHOD_NONE);
+    HttpRequestMethod a;
+    a.HttpRequestMethodXXX(NULL);
+    CPPUNIT_ASSERT(a == Http::METHOD_NONE);
+
     /* parsing a literal should work */
-    CPPUNIT_ASSERT(HttpRequestMethod("GET") == Http::METHOD_GET);
-    CPPUNIT_ASSERT(HttpRequestMethod("QWERTY") == Http::METHOD_OTHER);
+    HttpRequestMethod b;
+    b.HttpRequestMethodXXX("GET");
+    CPPUNIT_ASSERT(b == Http::METHOD_GET);
+    CPPUNIT_ASSERT_EQUAL(SBuf("GET"), b.image());
+    HttpRequestMethod c;
+    c.HttpRequestMethodXXX("QWERTY");
+    CPPUNIT_ASSERT(c == Http::METHOD_OTHER);
+    CPPUNIT_ASSERT_EQUAL(SBuf("QWERTY"), c.image());
+
+    // parsing error should not leave stale results
+    b.HttpRequestMethodXXX(NULL);
+    CPPUNIT_ASSERT(b == Http::METHOD_NONE);
+    CPPUNIT_ASSERT_EQUAL(SBuf("NONE"), b.image());
 }
 
 /*
- * We can also parse precise ranges of characters
+ * We can also parse precise ranges of characters with SBuf
  */
 void
 testHttpRequestMethod::testConstructCharStartEnd()
 {
     char const * buffer;
     /* parse an empty string -> Http::METHOD_NONE */
-    CPPUNIT_ASSERT(HttpRequestMethod(NULL) == Http::METHOD_NONE);
+    CPPUNIT_ASSERT(HttpRequestMethod(SBuf()) == Http::METHOD_NONE);
     /* parsing a literal should work */
-    CPPUNIT_ASSERT(HttpRequestMethod("GET") == Http::METHOD_GET);
+    CPPUNIT_ASSERT(HttpRequestMethod(SBuf("GET")) == Http::METHOD_GET);
     /* parsing with an explicit end should work */
     buffer = "POSTPLUS";
     CPPUNIT_ASSERT(HttpRequestMethod(SBuf(buffer, 4)) == Http::METHOD_POST);
@@ -90,15 +115,15 @@ testHttpRequestMethod::testImage()
 {
     // relaxed RFC-compliance parse HTTP methods are upgraded to correct case
     Config.onoff.relaxed_header_parser = 1;
-    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod("POST").image());
-    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod("pOsT").image());
-    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod("post").image());
+    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod(SBuf("POST")).image());
+    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod(SBuf("pOsT")).image());
+    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod(SBuf("post")).image());
 
     // strict RFC-compliance parse HTTP methods are case sensitive
     Config.onoff.relaxed_header_parser = 0;
-    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod("POST").image());
-    CPPUNIT_ASSERT_EQUAL(SBuf("pOsT"), HttpRequestMethod("pOsT").image());
-    CPPUNIT_ASSERT_EQUAL(SBuf("post"), HttpRequestMethod("post").image());
+    CPPUNIT_ASSERT_EQUAL(SBuf("POST"), HttpRequestMethod(SBuf("POST")).image());
+    CPPUNIT_ASSERT_EQUAL(SBuf("pOsT"), HttpRequestMethod(SBuf("pOsT")).image());
+    CPPUNIT_ASSERT_EQUAL(SBuf("post"), HttpRequestMethod(SBuf("post")).image());
 }
 
 /*
@@ -135,12 +160,12 @@ testHttpRequestMethod::testStream()
     // relaxed RFC-compliance parse HTTP methods are upgraded to correct case
     Config.onoff.relaxed_header_parser = 1;
     std::ostringstream buffer;
-    buffer << HttpRequestMethod("get");
+    buffer << HttpRequestMethod(SBuf("get"));
     CPPUNIT_ASSERT_EQUAL(String("GET"), String(buffer.str().c_str()));
 
     // strict RFC-compliance parse HTTP methods are case sensitive
     Config.onoff.relaxed_header_parser = 0;
     std::ostringstream buffer2;
-    buffer2 << HttpRequestMethod("get");
+    buffer2 << HttpRequestMethod(SBuf("get"));
     CPPUNIT_ASSERT_EQUAL(String("get"), String(buffer2.str().c_str()));
 }