Polish from branch audit

diff --git a/src/client_side.cc b/src/client_side.cc
index 179e75e8198882ce79fecea90cb74f561473209b..acbd3e78dcb2fb99c641d861c917d240dc87c332 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2868,7 +2868,7 @@ ConnStateData::clientParseRequests()
         /* Process request */
         ClientSocketContext *context = parseHttpRequest(this, *parser_);
         if (parser_->messageOffset()) {
-            // nothing but prefix garbage in the buffer. consume it.
+            // we are done with some of the buffer. consume it.
             connNoteUseOfBuffer(this, parser_->messageOffset());
             parser_->noteBufferShift(parser_->messageOffset());
         }

diff --git a/src/http/Http1Parser.cc b/src/http/Http1Parser.cc
index e3dfc2b1234d1143aacfad6c777ec1e8384d6df3..98c91787b8512c5d7c07d56e2a9d3fe6558782a6 100644 (file)
--- a/src/http/Http1Parser.cc
+++ b/src/http/Http1Parser.cc
@@ -7,7 +7,7 @@
 #include "SquidConfig.h"
 
 void
-Http1::Parser::clear()
+Http::One::Parser::clear()
 {
     completedState_ = HTTP_PARSE_NONE;
     buf = NULL;
@@ -18,7 +18,7 @@ Http1::Parser::clear()
 }
 
 void
-Http1::RequestParser::clear()
+Http::One::RequestParser::clear()
 {
     Http1::Parser::clear();
 
@@ -31,7 +31,7 @@ Http1::RequestParser::clear()
 }
 
 void
-Http1::Parser::reset(const char *aBuf, int len)
+Http::One::Parser::reset(const char *aBuf, int len)
 {
     clear(); // empty the state.
     completedState_ = HTTP_PARSE_NEW;
@@ -41,41 +41,17 @@ Http1::Parser::reset(const char *aBuf, int len)
 }
 
 void
-Http1::RequestParser::noteBufferShift(int64_t n)
+Http::One::RequestParser::noteBufferShift(int64_t n)
 {
-    bufsiz -= n;
-
     // if parsing done, ignore buffer changes.
     if (completedState_ == HTTP_PARSE_DONE)
         return;
 
-    // shift the parser resume point to match buffer content
+    // shift the parser resume point to match buffer content change
     parseOffset_ -= n;
 
-#if WHEN_INCREMENTAL_PARSING
-
-    // if have not yet finished request-line
-    if (completedState_ == HTTP_PARSE_NEW) {
-        // check for and adjust the known request-line offsets.
-
-        /* TODO: when the first-line is parsed incrementally we
-         * will need to recalculate the offsets for req.*
-         * For now, they are all re-calculated based on parserOffset_
-         * with each parse attempt.
-         */
-    }
-
-    // if finished request-line but not mime header
-    // adjust the mime header states
-    if (completedState_ == HTTP_PARSE_FIRST) {
-        /* TODO: when the mime-header is parsed incrementally we
-         * will need to store the initial offset of mime-header block
-         * instead of locatign it from req.end or parseOffset_.
-         * Since req.end may no longer be valid, and parseOffset_ may
-         * have moved into the mime-block interior.
-         */
-    }
-#endif
+    // and remember where to stop before performing buffered data overreads
+    bufsiz -= n;
 }
 
 /**
@@ -97,7 +73,7 @@ Http1::RequestParser::noteBufferShift(int64_t n)
  * \return true if garbage whitespace was found
  */
 bool
-Http1::RequestParser::skipGarbageLines()
+Http::One::RequestParser::skipGarbageLines()
 {
     req.start = parseOffset_; // avoid re-parsing any portion we managed to complete
 
@@ -150,7 +126,7 @@ Http1::RequestParser::skipGarbageLines()
  * \retval  0  more data is needed to complete the parse
  */
 int
-Http1::RequestParser::parseRequestFirstLine()
+Http::One::RequestParser::parseRequestFirstLine()
 {
     int second_word = -1; // track the suspected URI start
     int first_whitespace = -1, last_whitespace = -1; // track the first and last SP byte
@@ -383,18 +359,17 @@ Http1::RequestParser::parseRequestFirstLine()
 }
 
 bool
-Http1::RequestParser::parse()
+Http::One::RequestParser::parse()
 {
-    // stage 1: locate the request-line
     if (completedState_ == HTTP_PARSE_NEW) {
+
+        // stage 1: locate the request-line
         if (skipGarbageLines() && (size_t)bufsiz < parseOffset_)
             return false;
-    }
 
-    // stage 2: parse the request-line
-    if (completedState_ == HTTP_PARSE_NEW) {
+        // stage 2: parse the request-line
         PROF_start(HttpParserParseReqLine);
-        int retcode = parseRequestFirstLine();
+        const int retcode = parseRequestFirstLine();
         debugs(74, 5, "request-line: retval " << retcode << ": from " << req.start << "->" << req.end << " " << Raw("line", &buf[req.start], req.end-req.start));
         debugs(74, 5, "request-line: method " << req.m_start << "->" << req.m_end << " (" << method_ << ")");
         debugs(74, 5, "request-line: url " << req.u_start << "->" << req.u_end << " (" << uri_ << ")");
@@ -449,7 +424,7 @@ Http1::RequestParser::parse()
 #define GET_HDR_SZ     1024
 
 char *
-Http1::Parser::getHeaderField(const char *name)
+Http::One::Parser::getHeaderField(const char *name)
 {
     LOCAL_ARRAY(char, header, GET_HDR_SZ);
     const char *p = NULL;

diff --git a/src/http/Http1Parser.h b/src/http/Http1Parser.h
index 86439e440a4a7acc914b39222489a4887e0087f9..1169b2a60ef0d4bc56ea48bf23e1f77b14300b55 100644 (file)
--- a/src/http/Http1Parser.h
+++ b/src/http/Http1Parser.h
@@ -12,11 +12,13 @@ namespace Http {
 namespace One {
 
 // Parser states
-#define HTTP_PARSE_NONE   0 // nothing. completely unset state.
-#define HTTP_PARSE_NEW    1 // initialized, but nothing usefully parsed yet.
-#define HTTP_PARSE_FIRST  2 // have parsed request first line
-#define HTTP_PARSE_MIME   3 // have located end of mime header block
-#define HTTP_PARSE_DONE   99 // have done with parsing so far
+enum ParseState {
+    HTTP_PARSE_NONE =0,  ///< nothing. completely unset state.
+    HTTP_PARSE_NEW =1,   ///< initialized, but nothing usefully parsed yet
+    HTTP_PARSE_FIRST,    ///< HTTP/1 message first line
+    HTTP_PARSE_MIME,     ///< mime header block
+    HTTP_PARSE_DONE      ///< completed with parsing a full request header
+};
 
 /** HTTP protocol parser.
  *
@@ -50,7 +52,7 @@ public:
      * The leftmost n bytes bytes have been dropped and all other
      * bytes shifted left n positions.
      */
-    virtual void noteBufferShift(int64_t n) = 0;
+    virtual void noteBufferShift(const int64_t n) = 0;
 
     /** Whether the parser is already done processing the buffer.
      * Use to determine between incomplete data and errors results
@@ -77,6 +79,8 @@ public:
     const char *rawHeaderBuf() {return mimeHeaderBlock_.c_str();}
 
     /// attempt to parse a message from the buffer
+    /// \retval true if a full message was found and parsed
+    /// \retval false if incomplete, invalid or no message was found
     virtual bool parse() = 0;
 
     /// the protocol label for this message
@@ -93,7 +97,7 @@ public:
 
 protected:
     /// what stage the parser is currently up to
-    uint8_t completedState_;
+    ParseState completedState_;
 
     /// what protocol label has been found in the first line
     AnyP::ProtocolVersion msgProtocol_;
@@ -120,7 +124,7 @@ public:
     RequestParser() : Parser() {}
     RequestParser(const char *aBuf, int len) : Parser(aBuf, len) {}
     virtual void clear();
-    virtual void noteBufferShift(int64_t n);
+    virtual void noteBufferShift(const int64_t n);
     virtual int64_t messageOffset() const {return req.start;}
     virtual int64_t firstLineSize() const {return req.end - req.start + 1;}
     virtual bool parse();

diff --git a/src/tests/testHttp1Parser.cc b/src/tests/testHttp1Parser.cc
index cefd3ac9925881429fa1f4a6216287ec3643d567..20ca78987abd147427311628ccdb59a5f73a92bc 100644 (file)
--- a/src/tests/testHttp1Parser.cc
+++ b/src/tests/testHttp1Parser.cc
@@ -99,7 +99,7 @@ testHttp1Parser::testParseRequestLineProtocols()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -124,7 +124,7 @@ testHttp1Parser::testParseRequestLineProtocols()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -150,7 +150,7 @@ testHttp1Parser::testParseRequestLineProtocols()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -399,7 +399,7 @@ testHttp1Parser::testParseRequestLineStrange()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -425,7 +425,7 @@ testHttp1Parser::testParseRequestLineStrange()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -451,7 +451,7 @@ testHttp1Parser::testParseRequestLineStrange()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-5, output.req.end);
@@ -488,7 +488,7 @@ testHttp1Parser::testParseRequestLineTerminators()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -541,7 +541,7 @@ testHttp1Parser::testParseRequestLineTerminators()
         // Being tolerant we can ignore and elide these apparently benign CR
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -702,7 +702,7 @@ testHttp1Parser::testParseRequestLineMethods()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -728,7 +728,7 @@ testHttp1Parser::testParseRequestLineMethods()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -754,7 +754,7 @@ testHttp1Parser::testParseRequestLineMethods()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -824,7 +824,7 @@ testHttp1Parser::testParseRequestLineMethods()
         Config.onoff.relaxed_header_parser = 1;
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(1, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -874,7 +874,7 @@ testHttp1Parser::testParseRequestLineMethods()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -938,7 +938,7 @@ testHttp1Parser::testParseRequestLineInvalid()
         Config.onoff.relaxed_header_parser = 1;
         CPPUNIT_ASSERT_EQUAL(true, output.parse());
         CPPUNIT_ASSERT_EQUAL(true, output.isDone());
-//        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_NEW, output.completedState_);
+//        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_NEW, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(1, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -988,7 +988,7 @@ testHttp1Parser::testParseRequestLineInvalid()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -1036,7 +1036,7 @@ testHttp1Parser::testParseRequestLineInvalid()
         output.reset(input.content(), input.contentSize());
         CPPUNIT_ASSERT_EQUAL(false, output.parse());
         CPPUNIT_ASSERT_EQUAL(false, output.isDone());
-        CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, output.completedState_);
+        CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, output.completedState_);
         CPPUNIT_ASSERT_EQUAL(Http::scOkay, output.request_parse_status);
         CPPUNIT_ASSERT_EQUAL(0, output.req.start);
         CPPUNIT_ASSERT_EQUAL((int)input.contentSize()-1, output.req.end);
@@ -1215,7 +1215,7 @@ testHttp1Parser::testDripFeed()
             CPPUNIT_ASSERT_EQUAL(garbageEnd, (int)hp.parseOffset_);
             CPPUNIT_ASSERT_EQUAL(false, parseResult);
             CPPUNIT_ASSERT_EQUAL(false, hp.isDone());
-            CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_NEW, hp.completedState_);
+            CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_NEW, hp.completedState_);
             continue;
         }
 
@@ -1225,7 +1225,7 @@ testHttp1Parser::testDripFeed()
             CPPUNIT_ASSERT_EQUAL(false, parseResult);
             CPPUNIT_ASSERT_EQUAL(false, hp.isDone());
             // TODO: add all the other usual tests for request-line details
-            CPPUNIT_ASSERT_EQUAL((uint8_t)HTTP_PARSE_FIRST, hp.completedState_);
+            CPPUNIT_ASSERT_EQUAL(Http1::HTTP_PARSE_FIRST, hp.completedState_);
             continue;
         }