Re: [libvirt] [PATCH] buf: Fix possible infinite loop in EscapeString, VSnprintf
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org