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

Eric Blake eblake at redhat.com
Wed Sep 1 20:38:25 UTC 2010


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).

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list