Re: API inconsistency in HttpHeader.cc?

 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.

 Empty CC is invalid HTTP. If you are creating an empty CC header
 something is wrong elsewhere in the code.

 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.

 Amos
Received on Mon Oct 03 2011 - 22:42:44 MDT