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
