Re: API inconsistency in HttpHeader.cc?

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.

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

HTH,

Alex.
Received on Mon Oct 03 2011 - 16:09:56 MDT