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

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

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

-    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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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