Improve handling of problematic chunked-encoding trailers

[rousskov]

Squid misinterpreted certain chunked-encoded HTTP requests, leading to
framing errors. Chunked HTTP responses were similarly affected, but only
where read_ahead_gap was configured to exceed 64 KB. ICAP response
parsing code had a similar flaw but appears to be protected by a
hard-coded 64 KB TCP receive buffer size limit. ICAP trailers (see 2016
commit 69c698a) were not affected.

Background: The ChunkedCodingParser class used C++ exceptions to
report parsing errors. Its HTTP and ICAP users relied on that mechanism.
2015 commit 350ec67 shoveled that class into Http1::Parser hierarchy
but did not update its parsing method to match Http1::Parser API that
uses a combination of false parse() and false needsMoreData()
(instead of C++ exceptions) to report parsing errors. The follow-up
commit be29ee3 then upgraded ChunkedCodingParser trailer parsing code
to call Http1::Parser::grabMimeBlock() that used Http1::Parser error
reporting API, breaking all ChunkedCodingParser users (that continued
to rely exclusively on C++ exceptions for parsing error detection).

This change wraps that grabMimeBlock() call to provide exception-based
error reporting that all chunked transfer coding users still expect.
Those users now detect and handle trailer parsing errors. Also, Squid
now tags affected HTTP messages by setting %err_detail to
BAD_CHUNKED_REQUEST_BODY or BAD_CHUNKED_RESPONSE_BODY.

The same 2015 work also altered ChunkedCodingParser::needsMoreData()
meaning without updating its callers. This change updates the callers to
assert the expected needsMoreData() state after a non-throwing (i.e.
no error) parse() call. These new assertions do not change traffic
handling in known cases, but they may help prevent (or improve handling
of) yet-unknown parsing bugs similar to the one fixed here.

Also dropped two needsMoreSpace() calls that those 2015 changes made
misleading and polished Http::One::TeChunkedParser::parseChunkBody()
to return false when the method cannot finish parsing the chunk body.
All other similar helper parsing methods in that class already do that.

TODO: This change preserves the clash of error-reporting mechanisms in
Http::One::Parser parent class and its TeChunkedParser child.
Properly addressing that conflict is not necessary for fixing in-scope
parsing problems, and it requires making controversial design decisions
(and a lot more code changes).

[rousskov]

To see all contexts where overloaded Http::One::Parser::parse() is called, look at branch commit f1a3910 diff. That temporary code is not visible in this PR diff, but it clearly shows two kinds of Http::One::Parser::parse() callers:

• parse2() callers rely on the original Http1::Parser API that uses a combination of false parse() and false needsMoreData() to detect parsing errors.
• parseOrThrowXXX() callers rely on the original ChunkedCodingParser API that uses C++ exceptions to detect parsing errors.

This PR fixes Http::One::TeChunkedParser::parse() (i.e. parseOrThrowXXX() cases) to throw those exceptions, as its callers expect.

TeChunkedParser still does not faithfully implement its parent class API, but fixing that design problem would require a lot more changes, and I bet that some of those changes would be controversial. Let's fix the "immediate" parsing bug first.

[yadij]

Why throw instead of waiting for more data that might make parsedOk true?

[rousskov]

Why throw instead of waiting for more data that might make parsedOk true?

This code throws when parsedOk is already true, not when it is still false and "more data that might make parsedOk true". In other words, this code is "waiting for more data that might make parsedOk true".

This code throws when parsedOk is true here because this code snippet relies on 'true needsMoreData() implies "Incomplete response"' implication. One could argue that this code should have tested parsedOk first, but those arguments are outside this PR scope. This PR does not change this code logic or structure. We just add an assertion in hope to catch future bugs similar to the one fixed in this PR.

As this review question illustrates, this logic is tricky and dangerous, but this PR does not change any of that.

Finally, please note that there is an equivalent assertion a few lines above already. We are just improving assertion coverage here. Here is the abbidged version of this code showing both old/official and new/PR assertions:

if (hp->needsMoreData()) {
    if (eof) {
        assert(!parsedOk); // old
        // fall through to handle this premature EOF as an error
    } else {
        Assure(!parsedOk); // new (previously missing)
        return;
    }
}

If you insist, I can remove the old assertion and move the new one above the eof check:

if (hp->needsMoreData()) {
    Assure(!parsedOk); // covers both cases in this position
    if (!eof) {
        ; // fall through to handle this premature EOF as an error
    } else {
        return;
    }
}

[yadij]

Why this conditional change?
The API can now produce the "successful parse COMPLETED" signal (aka true) if the entire buffer fills up before the parse completes.

[rousskov]

Why this conditional change?

While the spelling of this condition has changed, the condition remains the same (i.e. it produces the same true or false result in any valid parser state) because !needsMoreData() implies !needsMoreSpace(). In other words, when needsMoreData() is false, needsMoreSpace() is guaranteed to be false:

bool needsMoreData()  { return parsingStage_ != Http1::HTTP_PARSE_DONE; }
bool needsMoreSpace() { return parsingStage_ == Http1::HTTP_PARSE_CHUNK && ...; }

As mentioned in this PR description, 2015 commit 350ec67 made this code misleading: Prior to those changes, the two conditions -- "need additional bytes" and "need larger space" -- resided in an independent parser and were independent; ChunkedCodingParser::parse() returned true when both of them were false (i.e. when the parser did not need anything and, hence, was "done"). After those changes, now-inherited TeChunkedParser::needsMoreData() meaning has effectively changed from "need additional bytes" to "still parsing" or "want more parse() calls", which covers both "need additional bytes" and "need larger space" conditions. Current Http::One::TeChunkedParser() API/hierarchy is bad, but addressing those problems is outside this bug-fixing PR scope because it requires many more code changes, some of which are likely to be controversial.

The API can now produce the "successful parse COMPLETED" signal (aka true) if the entire buffer fills up before the parse completes.

The above assertion is false: "If the entire buffer fills up", then needsMoreData() still returns true, and, hence, this parse() method returns false rather than "true". Note how TeChunkedParser::parseChunkBody() preserves parsingStage_ when potentialSpaceSize is zero (i.e. when there are chunk bytes to parse but there is no space).