[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