One of our users uncovered a nasty bug in 3.1 today. Squid hangs on some
simple requests.
On investigating I found that an update to make it return errors had
used the wrong result code in a few places. Causing it to loop trying to
read more data and complete the first line which was already complete.
The parser function also has no unit tests to verify correct operation.
Included in this patch is a draft outline for some unit-tests.
If anyone has suggestions or knowledge of other input cases please speak
up; good, bad AND ugly examples wanted.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.7
Beta testers wanted for 3.2.0.1
=== modified file 'src/HttpMsg.cc'
--- src/HttpMsg.cc 2010-08-24 20:35:02 +0000
+++ src/HttpMsg.cc 2010-08-29 06:17:10 +0000
@@ -394,15 +394,28 @@
void
HttpParserInit(HttpParser *hdr, const char *buf, int bufsiz)
{
+ hdr->clear();
hdr->state = 1;
- hdr->request_parse_status = HTTP_STATUS_NONE;
hdr->buf = buf;
hdr->bufsiz = bufsiz;
- hdr->req_start = hdr->req_end = -1;
- hdr->hdr_start = hdr->hdr_end = -1;
debugs(74, 5, "httpParseInit: Request buffer is " << buf);
}
+void
+HttpParser::clear()
+{
+ state = 0; // ?
+ buf = NULL;
+ bufsiz = 0;
+ req_start = req_end = -1;
+ hdr_start = hdr_end = -1;
+ m_start = m_end = -1;
+ u_start = u_end = -1;
+ v_start = v_end = -1;
+ v_maj = v_min = 0;
+ request_parse_status = HTTP_STATUS_NONE;
+}
+
#if MSGDODEBUG
/* XXX This should eventually turn into something inlined or #define'd */
int
@@ -484,9 +497,15 @@
}
}
if (hmsg->req_end == -1) {
- retcode = 0;
- goto finish;
+ PROF_stop(HttpParserParseReqLine);
+ debugs(74, 5, "Parser: retval 0: from " << hmsg->req_start <<
+ "->" << hmsg->req_end << ": needs more data to complete first line.");
+ return 0; // This is the only spot in this function where (0) is valid result.
}
+
+ // NP: we have now seen EOL, more-data (0) cannot occur.
+ // From here on any failure is -1, success is 1
+
assert(hmsg->buf[hmsg->req_end] == '\n');
/* Start at the beginning again */
i = 0;
@@ -494,7 +513,7 @@
/* Find first non-whitespace - beginning of method */
for (; i < hmsg->req_end && (xisspace(hmsg->buf[i])); i++);
if (i >= hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
hmsg->m_start = i;
@@ -503,7 +522,7 @@
/* Find first whitespace - end of method */
for (; i < hmsg->req_end && (! xisspace(hmsg->buf[i])); i++);
if (i >= hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
hmsg->m_end = i - 1;
@@ -511,7 +530,7 @@
/* Find first non-whitespace - beginning of URL+Version */
for (; i < hmsg->req_end && (xisspace(hmsg->buf[i])); i++);
if (i >= hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
hmsg->u_start = i;
@@ -534,7 +553,7 @@
}
}
if (i > hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
@@ -555,7 +574,7 @@
/* XXX why <= vs < ? I do need to really re-audit all of this ..*/
for (i = last_whitespace; i <= hmsg->req_end && xisspace(hmsg->buf[i]); i++);
if (i > hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
@@ -581,17 +600,17 @@
goto finish;
}
if (i >= hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
/* next should be .; we -have- to have this as we have a whole line.. */
if (hmsg->buf[i] != '.') {
- retcode = 0;
+ retcode = -1;
goto finish;
}
if (i + 1 >= hmsg->req_end) {
- retcode = 0;
+ retcode = -1;
goto finish;
}
@@ -631,4 +650,3 @@
return retcode;
}
-
=== modified file 'src/HttpMsg.h'
--- src/HttpMsg.h 2010-08-19 00:12:43 +0000
+++ src/HttpMsg.h 2010-08-29 03:04:53 +0000
@@ -130,6 +130,10 @@
class HttpParser
{
public:
+ /// reset all fields to empty values.
+ void clear();
+
+public:
char state;
const char *buf;
int bufsiz;
=== modified file 'src/tests/testHttpRequest.cc'
--- src/tests/testHttpRequest.cc 2010-08-16 14:47:39 +0000
+++ src/tests/testHttpRequest.cc 2010-08-29 04:03:49 +0000
@@ -199,3 +199,338 @@
input.reset();
error = HTTP_STATUS_NONE;
}
+
+void
+testHttpRequest::testParseRequestLine()
+{
+ MemBuf input;
+ HttpParser output;
+ size_t hdr_len;
+ input.init();
+
+ // Valid States
+
+ // HTTP/0.9 simple-request
+ input.append("GET /\r\n", 7);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 9);
+// TODO : check "GET"
+// TODO : check "/"
+ input.reset();
+ output.clear();
+
+ // HTTP/1.0 full-request
+ input.append("GET / HTTP/1.0\r\n", 16);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+// TODO : check "GET"
+// TODO : check "/"
+ input.reset();
+ output.clear();
+
+ // HTTP/1.1 full-request
+ input.append("GET / HTTP/1.1\r\n", 16);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+// TODO : check "GET"
+// TODO : check "/"
+ input.reset();
+ output.clear();
+
+ // future version full-request
+ input.append("GET / HTTP/10.12\r\n", 18);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 10);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 12);
+ input.reset();
+ output.clear();
+
+ // extra fluffy whitespace.
+ input.append("GET / HTTP/1.1\r\n", 21);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, 0);
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // additional data in buffer
+ input.append("GET / HTTP/1.1\nboo!", 24);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, 0);
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.contentSize() - 4);
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // alternative EOL sequence: NL-only
+ input.append("GET / HTTP/1.1\n", 15);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, 0);
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // alternative EOL sequence: double-NL-only
+ input.append("GET / HTTP/1.1\n\n", 16);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, 0);
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.contentSize() - 1);
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // . method
+ input.append(". / HTTP/1.1\n", 13);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, 0);
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.contentSize());
+// TODO : check "."
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // OPTIONS with * URL
+ input.append("OPTIONS * HTTP/1.1\n", 19);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "OPTIONS"
+// TODO : check "*"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // unknown method
+ input.append("HELLOWORLD / HTTP/1.1\n", 22);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "HELLOWORLD"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // This stage of the parser also accepts non-HTTP protocol names
+ input.append("GET / FOO/1.0\n", 14);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == 1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "FOO"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+
+// Error States.
+
+ // method-only
+ input.append("A\n", 2);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "A"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_start, -1);
+ CPPUNIT_ASSERT_EQUAL(output.v_end, -1);
+ input.reset();
+ output.clear();
+
+ // no method
+ input.append("/ HTTP/1.0\n", 11);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "/" (request)
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_start, -1);
+ CPPUNIT_ASSERT_EQUAL(output.v_end, -1);
+ input.reset();
+ output.clear();
+
+ // binary code in method (error or not?)
+ input.append("GET\0xA / HTTP/1.1\n", 16);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET\0xA"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 1);
+ input.reset();
+ output.clear();
+
+ // no URL (grammer otherwise correct)
+ input.append("GET HTTP/1.1\n", 14);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+ // no URL (grammer invalid)
+ input.append("GET HTTP/1.1\n", 13);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_start, -1);
+ CPPUNIT_ASSERT_EQUAL(output.v_end, -1);
+ input.reset();
+ output.clear();
+
+ // no protocol
+ input.append("GET HTTP/1.1\n", 14);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+ // no version
+ input.append("GET / HTTP/\n", 12);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+ // no major version
+ input.append("GET / HTTP/.1\n", 14);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+ // no minor version
+ input.append("GET / HTTP/1.\n", 14);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "GET"
+// TODO : check "/"
+// TODO : check "HTTP"
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 1);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+ // binary line
+ input.append("\0xA\0xB\0xC\0xD\0xE\0xF\n", 7);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+// TODO : check "\0xA\0xB\0xC\0xD\0xE\0xF"
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ input.reset();
+ output.clear();
+
+ // mixed whitespace line
+ input.append(" \t \r \n", 9);
+ HttpParserInit(&output, input.content(), input.contentSize());
+ CPPUNIT_ASSERT(HttpParserParseReqLine(&input, &output) == -1);
+ CPPUNIT_ASSERT_EQUAL(output.req_start, input.content());
+ CPPUNIT_ASSERT_EQUAL(output.req_end, input.content() + input.contentSize());
+ // other fields should be in unset state still...
+ CPPUNIT_ASSERT_EQUAL(output.v_maj, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_min, 0);
+ CPPUNIT_ASSERT_EQUAL(output.v_start, -1);
+ CPPUNIT_ASSERT_EQUAL(output.v_end, -1);
+ input.reset();
+ output.clear();
+}
=== modified file 'src/tests/testHttpRequest.h'
--- src/tests/testHttpRequest.h 2009-07-26 09:24:07 +0000
+++ src/tests/testHttpRequest.h 2010-08-29 02:50:26 +0000
@@ -15,6 +15,7 @@
CPPUNIT_TEST( testCreateFromUrl );
CPPUNIT_TEST( testIPv6HostColonBug );
CPPUNIT_TEST( testSanityCheckStartLine );
+ CPPUNIT_TEST( testParseRequestLine );
CPPUNIT_TEST_SUITE_END();
public:
@@ -25,6 +26,7 @@
void testCreateFromUrl();
void testIPv6HostColonBug();
void testSanityCheckStartLine();
+ void testParseRequestLine();
};
#endif
