Re: [libvirt] [PATCH] Allocate buffer to hold xend response
On 06/03/2010 04:51 PM, Jim Fehlig wrote:
>> + if (content_length > 0) {
>> ssize_t ret;
>>
>> - if ((unsigned int) content_length > (n_content + 1))
>> - content_length = n_content - 1;
>> + if (VIR_ALLOC_N(*content, content_length) < 0 ) {
>> + virReportOOMError();
>> + return -1;
>> + }
>>
>> - ret = sread(fd, content, content_length);
>> + ret = sread(fd, *content, content_length);
> The above removes the code that guarantees the content buffer is
> NUL-terminated, yet I don't see where the requirement for NUL-termination
> has been dropped.
I removed that since VIR_ALLOC_N fills the memory with zeros. Correct?
It can, but only if you used VIR_ALLOC_N(,,content_length + 1) in case
sread reads exactly content_length bytes, before you are still
guaranteed a trailing NUL. In other words, Jim is right that you need
to make an adjustment still.
> Also, now that we'll get the buffer length from an untrusted
> source, it would be good to ensure that it's not outrageously
> large before using it as the size of the buffer we allocate,
> assuming there is some reasonably low upper bound on the maximum
> payload size. That could save us from potential DoS abuse.
>
Good point. Suggestions on the limit?
4k is obviously too small, or you wouldn't have written this patch. Do
you know whether 64k is sufficient? On the other hand, even 1M as an
upper limit is probably still reasonable these days.
BTW, I pushed the patch after Eric's ACK. I'll role another to address
these issues, once I get confirmation on the NUL-termination.
Looking forward to the followup, and thanks for Jim for catching
something I missed in my ACK (even if we didn't catch it in time).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)