On 09/01/2010 04:38 PM, Eric Blake wrote:
> On 09/01/2010 01:43 PM, Cole Robinson wrote:
>> The current code will go into an infinite loop if the printf generated
>> string is>= 1000, AND exactly 1 character smaller than the amount of free
>> space in the buffer. When this happens, we are dropped into the loop body,
>> but nothing will actually change, because count == (buf->size - buf->use - 1),
>> and virBufferGrow returns unchanged if count< (buf->size - buf->use)
>>
>> Fix this by removing the '- 1' bit from 'size'. The *nprintf functions handle
>> the NULL byte for us anyways, so we shouldn't need to manually accomodate
>> for it.
>
> Per 'man vsnprintf',
>
>> The functions snprintf() and vsnprintf() do not write more than size
>> bytes (including the trailing '\0'). If the output was truncated due
>> to this limit then the return value is the number of characters (not
>> including the trailing '\0') which would have been written to the final
>> string if enough space had been available. Thus, a return value of
>> size or more means that the output was truncated. (See also below
>> under NOTES.)
>
> So, if the result is == size, then we MUST re-alloc and try again, to
> avoid truncation.
>
>> - size = buf->size - buf->use - 1;
>> + size = buf->size - buf->use;
>> va_start(argptr, format);
>> va_copy(locarg, argptr);
>> while (((count = vsnprintf(&buf->content[buf->use], size, format,
>
> This makes sense - we might as well tell vsnprintf exactly how many
> bytes it has to play with.
>
>> - locarg))< 0) || (count>= size - 1)) {
>> + locarg))< 0) || (count>= size)) {
>
> However, I think this still has issues. vsnprintf returns -1 ONLY when
> there is a non-recoverable error (EILSEQ, ENOMEM), and NOT when the
> string was truncated. So if it returns -1 once, it will ALWAYS return
> -1 no matter how much larger we make the buffer. (At least, since we
> use the gnulib module, it will always obey POSIX. If it weren't for
> gnulib, then yes, I could see how you could worry about older glibc that
> didn't obey POSIX and returned -1 instead of the potential print size).
> So why not write it as an if() instead of a while(), since we really
> only need one truncated vsnprintf attempt before guaranteeing that the
> buffer is large enough for the second attempt.
>
> [Side note: I'm really starting to get annoyed by the Thunderbird bug
> that corrupts spacing of quoted text that contains & or <. Sorry to
> everyone else that has to put up with my review comments that have been
> mangled.]
>
>> @@ -250,13 +250,12 @@ virBufferVSprintf(const virBufferPtr buf, const char *format, ...)
>> return;
>> }
>>
>
> You're missing a step. It's okay that size is 1 larger, but you also
> need to make the virBufferGrow(buf, grow_size) guarantee that it
> allocates at least count+1 bytes, to avoid a needless second iteration
> through the loop. Back to that comment of an if() being better than a
> while() loop.
>
>> - size = buf->size - buf->use - 1;
>> + size = buf->size - buf->use;
>> va_copy(locarg, argptr);
>> }
>> va_end(argptr);
>> va_end(locarg);
>> buf->use += count;
>> - buf->content[buf->use] = '\0';
>
> Agree with this part.
>
>
>> }
>>
>> /**
>> @@ -340,19 +339,18 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st
>> return;
>> }
>>
>> - size = buf->size - buf->use - 1;
>> + size = buf->size - buf->use;
>> while (((count = snprintf(&buf->content[buf->use], size, format,
>> - (char *)escaped))< 0) || (count>= size - 1)) {
>> + (char *)escaped))< 0) || (count>= size)) {
>> buf->content[buf->use] = 0;
>> grow_size = (count> 1000) ? count : 1000;
>> if (virBufferGrow(buf, grow_size)< 0) {
>
> Same problem on while() vs. if(), and not allocating a large enough buffer.
>
> NACK as written, but this is indeed a serious bug, so looking forward to
> v2 (I can help you write it, if needed).
>
THanks for the review (online and offline). I've sent an updated patch
with your recommended improvements.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list