[libvirt] [PATCH] buf: Fix possible infinite loop in EscapeString, VSnprintf

[Cole Robinson]

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