From: Amos Jeffries Date: Wed, 15 Oct 2014 14:09:32 +0000 (-0700) Subject: Audit updates X-Git-Tag: merge-candidate-3-v1~506^2~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f96881323e8306c672c335e2fd7e1e71e9754c3f;p=thirdparty%2Fsquid.git 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 acc944975d..56628f120b 100644 --- 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 295bd0639a..16cd9c8757 100644 --- 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 *q = new CbDataList (HttpRequestMethod(t)); + HttpRequestMethod m; + m.HttpRequestMethodXXX(t); + CbDataList *q = new CbDataList(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 1f0625d7bc..82b17ea58f 100644 --- 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(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 e430e67cd2..3e356eee2a 100644 --- 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 02703a8e7a..0c6b0b28df 100644 --- 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 826413510b..a95861fc52 100644 --- 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 63c9318c18..2c9c584b4e 100644 --- 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 3226be5c75..64cd6616c5 100644 --- 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 d4693b391e..c72b54dc89 100644 --- 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 927f5ff6dd..6865b30b65 100644 --- 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 742bab8039..7bfff5ec03 100644 --- 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 a541f39421..4b94d2606f 100644 --- 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())); }