[squid-dev] [PATCH] Incorrect processing of long URIs
rousskov at measurement-factory.com
Tue Aug 23 14:50:18 UTC 2016
On 08/23/2016 03:26 AM, Eduard Bagdasaryan wrote:
> 2016-08-23 3:08 GMT+03:00 Alex Rousskov:
>> I do not understand why you decided to use maxMethodLength in
>> parseRequestFirstLine(). AFAICT, parseMethodField() already does
>> everything we need: It logs an error message and sets parseStatusCode
> Yes, it does partly what we need but it does not parse the delimiter(
> and inform about possible failure).
... which is unrelated to the maxMethodLength move I questioned above.
> skipDelimiter() does this with message:
>> invalid request-line: missing delimiter
> which does not hint that our case is a 'bad' method
If the method parses OK but the delimiter is missing, then our case can
be considered to be a case of a "missing delimiter" rather than of a
"bad method". It is all relative, of course: If a request line starts
with a few token characters, then it is impossible to say whether the
method suffix is invalid or the following delimiter -- it takes two to
> and less informative than:
>> invalid request-line: method exceeds 32-byte limit
How is a "missing delimiter" error is less informative than "method
exceeds 32-byte limit", especially when the method does not actually
exceed that limit? [Rhetorical]
> I followed your sketch and refreshed the patch.
> + debugs(74, ErrorLevel(), "invalid request-line exceeds " <<
> + Config.maxRequestHeaderSize << "-byte limit");
bzr grep "invalid request-line" src
s/request-line/request-line: URI/ for consistency and clarity sake.
> + const CharacterSet &delimiters = DelimiterCharacters();
I wonder whether we should make this variable static to avoid repeated
function calls on a performance-sensitive code path. Same for the old
"delimiters" variable left inside parseRequestFirstLine(), of course.
These polishing touches can be done during commit.
More information about the squid-dev