]> git.ipfire.org Git - thirdparty/squid.git/commit
Bug 4368: A simpler and more robust HTTP request line parser.
authorAlex Rousskov <rousskov@measurement-factory.com>
Wed, 18 Nov 2015 23:56:16 +0000 (15:56 -0800)
committerAmos Jeffries <squid3@treenet.co.nz>
Wed, 18 Nov 2015 23:56:16 +0000 (15:56 -0800)
commite02f963ce8082f66bc3656e16112f91d5b937d06
tree7959a374441b05ebe60bc4f44403f97d1a0b9a56
parent895171b10551ccb5ab59b56a967c5d95cb5e439c
Bug 4368: A simpler and more robust HTTP request line parser.

The primary changes are: Removed incremental parsing and revised parsing
sequence to accept virtually any URI (by default and also configurable
as before).

Also doubled hard-coded 16-character method length limit.

No changes to parsing HTTP header fields (a.k.a. the MIME block) were
intended.

Known side effects:

* Drastically simpler code.
* Some unit test case adjustments.
* The new parser no longer treats some request lines ending with
  "HTTP/1.1" as HTTP/0.9 requests for URIs that end with "HTTP/1.1".
* The new parser no longer re-allocates character sets while parsing
  each request.

Intentional Changes:

* Removal of incremental request line parsing.

Squid parsed the request line incrementally. That optimization was
unnecessary:
  - most request lines are short enough to fit into one network I/O,
  - the long lines contain only a single long field (the URI), and
  - the user code must not use incomplete parsing results anyway.

Incremental parsing made code much more complex and possibly slower than
necessary.

The only place where incremental parsing of request lines potentially
makes sense is the URI field itself, and only if we want to accept URIs
exceeding request buffer capacity. Neither the old code, nor the
simplified one do that right now.

* Accept virtually any request-target (when allowed).

1. relaxed_header_parser allows whitespace in request-target.
2. relaxed_header_parser combined with USE_HTTP_VIOLATIONS now allows
   any characters except non-whitespace CTL characters (see RFC 5234
   appendix B.1) in the message request-target (aka URI).

#2 being the default build and configuration situation allows virtually
any URI that Squid can isolate by stripping method (prefix) and
HTTP/version (suffix) off the request line. This approach allows Squid to
forward slightly malformed (in numerous ways) URIs instead of misplacing
on the Squid admin the burden of explaining why something does not work
going through Squid but works fine when going directly or through another
popular proxy (or through an older version of Squid!).

URIs in what Squid considers an HTTP/0.9 request obey the same rules.
Whether the rules should differ for HTTP/0 is debatable, but the current
implementation is the simplest possible one, and the code makes it easy
to add complex rules.

* Code simplification.

RequestParser::parseRequestFirstLine() is now a simple sequence of
sequential if statements. There is no longer a path dedicated for the
strict parser. The decisions about parsing individual fields and
delimiters are mostly isolated to the corresponding methods.

* Unit test cases adjustments.

Removal of incremental request line parsing means that we should not
check parsed fields when parsing fails or has not completed yet.

Some test cases made arguably weird decisions apparently to accommodate
the old parser. The expectations of those test cases are more natural now.

Also, added optional (and disabled by default) debugging, to help pin-point
failures to test sub-cases that CPPUNIT cannot see.

Changing request methods to "none" in test sub-cases with invalid input
was not technically necessary because the new code ignores the method
when parsing fails, but it may help whoever would decide to reduce test
code duplication (by replacing hand-written expected outcomes for failed
test cases with a constant assignment or function call).
src/http/one/RequestParser.cc
src/http/one/RequestParser.h
src/tests/testHttp1Parser.cc