[squid-dev] [PATCH] Incorrect processing of long URIs

[Alex Rousskov]

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
>> accordingly.

> 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
tango.


> 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.

+1

Alex.