[squid-dev] [PATCH] ICAP trailer support

[Amos Jeffries]

On 10/11/2016 7:29 a.m., Alex Rousskov wrote:
> On 11/09/2016 08:16 AM, Amos Jeffries wrote:
>> On 9/11/2016 3:05 a.m., Eduard Bagdasaryan wrote:
>>> Also simplified and fixed headers isolating code while dealing with
>>> empty (i.e. zero header fields) headers. Old httpMsgIsolateHeaders()
>>> tried to re-implement header end detection/processing logic that is
>>> actually covered by headersEnd(). Old httpMsgIsolateHeaders() could
>>> return success for some garbage input (e.g., a buffer of several CRs)
>>> even if no end of headers was found.
> 
> 
>> Careful there. Since HttpMsg is actually HTTP parse logic a buffer
>> containing several CR's can be a valid mime block (just empty). CR is
>> part of BWS/OWS in HTTP.
> 
> Amos,
> 
>     Do you agree that a buffer consisting of several CRs and nothing
> else must not result in httpMsgIsolateHeaders() returning 1 (i.e.
> "success")?

Only if all the callers of this function are handling the case where it
produces non-1 and adding a terminator LF explicitly for a re-parse attempt.

IIRC we made the HTTP code do that, but I'm not sure of the ICAP or HTCP
code, or the Store loading code (which parses into StoreEntry/MemObject
or something).

If this functions output in this case is going to change then all those
callers need to be checked that they cope with this change correctly.


> 
> The above preamble text is trying to say that the old
> httpMsgIsolateHeaders() could return 1 in that invalid-input case. The
> text should be rephrased for clarity if that is not how you interpreted
> it. Suggestions welcomed.
> 
> 
> In this context, the "HTTP header(s)" is defined as
> 
>   *( header-field CR?LF )
>   CR?LF
> 
> where CR?LF is either CRLF or just LF.
> 
> Old httpMsgIsolateHeaders() and new HttpHeader::Isolate() must extract
> the mime block (i.e., the *(header-field CR?LF) part) but only if
> headersEnd() finds the end of the HTTP header first. The old
> httpMsgIsolateHeaders was buggy. If you believe HttpHeader::Isolate() is
> buggy, please speak up!

I have not seen any bugs, just the above needs doing.

Amos