[squid-dev] [PATCH] Reject or sanitize more problematic Content-Length values

[Alex Rousskov]

On 09/02/2016 09:05 AM, Amos Jeffries wrote:
> On 2/09/2016 11:21 a.m., Alex Rousskov wrote:
>> This change handles multiple Content-Length values inside
>> one header field, negative values, and trailing garbage. Handling the
>> former required a change in the overall Content-Length interpretation
>> approach (which is why it was previously left as a TODO).


> +1. Looks okay to me. With one caveat:
> 
> Since there are several of these interpreters for special-cased headers
> I think we need to define a sub-namespace for header
> interpreters/parsers, etc. 

I am not sure I understand or agree with your logic for deciding when to
add a namespace. A namespace is primarily a name clash avoidance
mechanism. What name clashes are you trying to avoid by adding a "header
interpreters/parsers" namespace?

As an illustration, consider the standard library that provides a lot of
things, including a lot of similar things (e.g., stream classes or math
functions). How many namespaces does std contain? I knew of only a
couple before I looked it up. There are actually about a dozen
"standard" ones, but all the ones I checked are concerned with name
clashes, not just grouping similar things together.


> Does keeping the existing "Hdr" term seem like a good idea to you?

Not sure what you mean. Are you suggesting to rename
ContentLengthInterpreter to ContentLengthHdrInterpreter? That looks like
a clash of naming styles to me.


> * If so please move ContentLengthInterpreter to a Http::Hdr:: namespace
> and ContentLengthInterpreter.{h,cc} files in a matching directory.
> 
> * Otherwise, please at least place the class within the Http:: namespace
> and directory etc.

I recommend leaving ContentLengthInterpreter class name as is. The class
interprets Content-Length headers to find their intended meaning during
parsing. It is not meant to be an Http::ContentLength class that
stores/manages/writes parsed Content-Length information.

If that is not going to create linking problems, I can move the
ContentLengthInterpreter class into http/one/ContentLengthInterpreter.*
files and place it inside the existing Http::One namespace. Would that
be sufficient to address your concerns?


> (PS. I plan to bundle next 4.x within the next 2-3 days. I leave it to
> you to decide whether this gets applied now or after release).

I would prefer to commit now, but that requires reaching an agreement
regarding ContentLengthInterpreter name and location as discussed above.


Thank you,

Alex.