[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 09/09/2010 10:50 PM, Eric Blake wrote:
> On 09/09/2010 08:09 AM, 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 accommodate
>> for it.
>>
>> Here's a bug where we are actually hitting this issue:
>> https://bugzilla.redhat.com/show_bug.cgi?id=602772
>>
>> v2: Eric's improvements: while ->  if (), remove extra va_list variable,
>>      make sure we report buffer error if snprintf fails
>>
>> v3: Add tests/virbuftest which reproduces the infinite loop before this
>>      patch, works correctly after
> 
> I confirm that the only change in v3 is the addition of a test.
> 
>> +static int testBufInfiniteLoop(const void *data ATTRIBUTE_UNUSED)
>> +{
>> +    virBuffer bufinit = VIR_BUFFER_INITIALIZER;
>> +    virBufferPtr buf =&bufinit;
>> +    char *addstr = NULL, *bufret = NULL;
>> +    int ret = -1;
>> +    const struct testInfo *info = data;
>> +
>> +    virBufferAddChar(buf, 'a');
>> +    if (buf->a != 1002 || buf->b != 1) {
>> +        TEST_ERROR("Buf was not expected size, size=%d use=%d\n",
>> +                   buf->a, buf->b);
> 
> This seems a bit fragile; it is inspecting the innards of virBuffer. 
> Maybe add a comment here that any change to the innards of virBuffer may 
> warrant a change to this test at the same time.  But having the test, 
> even if it is fragile, is a good thing, so...
> 
> ACK.
>

Thanks, pushed with a comment.

- Cole

> We could add further tests to this file to cover more lines of code, 
> such as: printing a string that needs escapes added, and actually 
> verifying that the escaped string matches expectations; passing a bad 
> format string (such as "%$2s" to trigger EINVAL, 
> "%*d%*d",1<<31,1,1<<31,1 to trigger EOVERFLOW) and verifying that error 
> gets set.  But those can be in a separate patch; let's get the bug fix 
> in now before 0.8.4.
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]