Re: API inconsistency in HttpHeader.cc?

 On Mon, 03 Oct 2011 17:31:30 -0600, Alex Rousskov wrote:
> On 10/03/2011 04:42 PM, Amos Jeffries wrote:
>> On Mon, 03 Oct 2011 10:09:42 -0600, Alex Rousskov wrote:
>>> On 10/03/2011 08:37 AM, Kinkie wrote:
>>>> Hi all,
>>>> while working on playground (HttpHdrCc c++-ification, mostly), I
>>>> stumbled upon something which worries me a bit, and I wonder why
>>>> it is
>>>> not causing issues.
>>>>
>>>> HttpHeader.cc defines a few functions which add headers, I'm
>>>> zoning in
>>>> onto HttpHeader::putCc as is the one I've been looking at the
>>>> most.
>>>> Here it is:
>>>>
>>>> void
>>>> HttpHeader::putCc(const HttpHdrCc * cc)
>>>> {
>>>> MemBuf mb;
>>>> Packer p;
>>>> assert(cc);
>>>> /* remove old directives if any */
>>>> delById(HDR_CACHE_CONTROL);
>>>> /* pack into mb */
>>>> mb.init();
>>>> packerToMemInit(&p, &mb);
>>>> cc->packInto(&p);
>>>> /* put */
>>>> addEntry(new HttpHeaderEntry(HDR_CACHE_CONTROL, NULL,
>>>> mb.buf));
>>>> /* cleanup */
>>>> packerClean(&p);
>>>> mb.clean();
>>>> }
>>>>
>>>> The problem is that addEntry is initialized from the raw storage
>>>> of a
>>>> MemBuf (filled-in via packer), expecting a NULL-terminated
>>>> c-string
>>>> value.
>>>> Which may very well not be the case. For instance, if noone as
>>>> written
>>>> anything to the MemBuf.
>>>
>>> I suspect it was impossible for the old code to call putCc with an
>>> empty
>>> Cache-Control header under normal conditions. Abnormal conditions
>>> were
>>> rare, [nearly] never fatal because the buffer is [usually] zeroed
>>> on
>>> allocation, and so the bug was never noticed.
>
> I should have made my comments less specific to the empty CC header
> case. What Kinkie is asking about is null-termination which is not
> specific to empty CC. Kinkie just used an empty CC as an obvious
> example
> of a not explicitly terminated mb.buf.
>

 Ah, Right.

>
>> Empty CC is invalid HTTP. If you are creating an empty CC header
>> something is wrong elsewhere in the code.
>
> While it is true that empty CC on the wire is syntactically invalid,
> I
> can think of three cases where an object representing it may appear
> inside Squid:
>
> 1. Squid trying to preserve original headers instead of trying to
> "fix" them. I do not think we do that to CC now, but many proxy
> admins
> want their proxies to be minimally invasive. Yes, this creates
> smuggling-like exploitation opportunities but so is fixing headers
> (it
> is essentially two sides of the same coin).

 For this case I think we want to have a header-blob construct.
 Essentially what I'm expecting the HeaderParser to produce after SBuf
 conversion:
  - a 'parent' SBuf pointing to the start of the header line and running
 to terminal '\n'.
  - a 'label' SBuf pointing to the start of the header name and running
 to terminal ':'.
  - a 'header-value' SBuf pointing to the start of the non-whitespace
 header values and running to terminal '\n'.

 When preserving originals we decide whether to output the second two as
 a pair, like we are supposed to for compliant syntax cleaning, resulting
 in trimmed whitespace garbage. Or for fully transparent, dump the first
 back onto the wire.

>
> 2. Squid obeying configuration options or some kind of internal
> logic
> that removes an existing CC directive. I am not sure there are cases
> like that now, but that is not important for this thread. There is no
> code that checks that removing a directive does not lead to an empty
> cache control header. Moreover, I do not think we need such code as
> it
> would make callers life difficult.

 I think we may have this as a unreported problem. There was a comment
 in HTTPbis about proxies emitting empty headers questioning what to do
 about storage in that case. So likely there is a portion of the user
 base causing or experiencing this brokenness.

>
> 3. Debugging. We may print an "under construction" CC header that
> is
> currently empty. I doubt we do that now, but I doubt an audit would
> catch this if we start doing it later. And debugging code is not the
> place to check for header validity anyway.
>
> Until we start doing #1, we can simply check for an empty CC header
> when
> packing it to solve #2 and #3 (but this is not what Kinkie is asking
> about, I think).
>
>
>> Same for range and SC headers.
>>
>> In the case of SC replacing a CC with nothing there will still be a
>> '
>> field="" ' string value to set.
>>
>>>
>>>> A possible solution could be to mb.terminate() just before
>>>> addEntry,
>>>> but that has its own problems: if the MemBuf doesn't have the
>>>> space to
>>>> grow for appending the trailing \0, it will assert (see XXX on
>>>> MemBuff::terminate() ).
>>>>
>>>> putContRange, putRange, putSc all share the same blueprint and,
>>>> potentially, issue.
>>>>
>>>> I wonder why this is not biting us, and how to best address this.
>>>> Any
>>>> ideas or suggestions?
>>
>> I think the code which needs to just clear the header is being
>> compliant
>> and using delById() API instead of the put*() API with no value.
>>
>>>
>>> I suggest the following:
>>>
>>> 1) Add a safe convertion from MemBuf to String (the new explicit
>>> String constructor or a global function will have access to MemBuf
>>> length so it will not need to terminate the MemBuf).
>>>
>>> 2) Add a HttpHeaderEntry constructor that accepts const MemBuf
>>> reference as header value.
>>>
>>> This approach will not increase the number of copies we make, will
>>> not
>>> change overall header handling logic, and will avoid unterminated
>>> buffer
>>> use.
>>>
>>
>> This may be needed anyway, and/or a good idea. But I think its a fix
>> for
>> the wrong problem here.
>
> If I interpreted Kinkie's concern correctly, the situation is
> reversed:
> Empty CC is the "wrong problem" as he is concerned about not
> explicitly
> terminated buffer which may happen even if CC is not empty as packing
> API does not guarantee termination, IIRC. My suggestions are meant to
> address that concern only.

 Okay. I stand corrected.

 Amos
Received on Tue Oct 04 2011 - 01:18:12 MDT