buf: Fix possible infinite loop in EscapeString, VSnprintf

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

diff --git a/src/util/buf.c b/src/util/buf.c
index fc1217bf608c60390eee503499d0cc70e6e9680b..553e2a059e33a46ca1a3aefc1a2f13e15ee11e95 100644 (file)
--- a/src/util/buf.c
+++ b/src/util/buf.c
@@ -224,7 +224,7 @@ void
 virBufferVSprintf(const virBufferPtr buf, const char *format, ...)
 {
     int size, count, grow_size;
-    va_list locarg, argptr;
+    va_list argptr;
 
     if ((format == NULL) || (buf == NULL))
         return;
@@ -236,27 +236,38 @@ virBufferVSprintf(const virBufferPtr buf, const char *format, ...)
         virBufferGrow(buf, 100) < 0)
         return;
 
-    size = buf->size - buf->use - 1;
     va_start(argptr, format);
-    va_copy(locarg, argptr);
-    while (((count = vsnprintf(&buf->content[buf->use], size, format,
-                               locarg)) < 0) || (count >= size - 1)) {
+
+    size = buf->size - buf->use;
+    if ((count = vsnprintf(&buf->content[buf->use],
+                           size, format, argptr)) < 0) {
+        buf->error = 1;
+        goto err;
+    }
+
+    /* Grow buffer if necessary and retry */
+    if (count >= size) {
         buf->content[buf->use] = 0;
-        va_end(locarg);
+        va_end(argptr);
+        va_start(argptr, format);
 
-        grow_size = (count > 1000) ? count : 1000;
+        grow_size = (count + 1 > 1000) ? count + 1 : 1000;
         if (virBufferGrow(buf, grow_size) < 0) {
-            va_end(argptr);
-            return;
+            goto err;
         }
 
-        size = buf->size - buf->use - 1;
-        va_copy(locarg, argptr);
+        size = buf->size - buf->use;
+        if ((count = vsnprintf(&buf->content[buf->use],
+                               size, format, argptr)) < 0) {
+            buf->error = 1;
+            goto err;
+        }
     }
-    va_end(argptr);
-    va_end(locarg);
     buf->use += count;
-    buf->content[buf->use] = '\0';
+
+err:
+    va_end(argptr);
+    return;
 }
 
 /**
@@ -336,23 +347,34 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st
 
     if ((buf->use >= buf->size) &&
         virBufferGrow(buf, 100) < 0) {
-        VIR_FREE(escaped);
-        return;
+        goto err;
+    }
+
+    size = buf->size - buf->use;
+    if ((count = snprintf(&buf->content[buf->use], size,
+                          format, (char *)escaped)) < 0) {
+            buf->error = 1;
+        goto err;
     }
 
-    size = buf->size - buf->use - 1;
-    while (((count = snprintf(&buf->content[buf->use], size, format,
-                              (char *)escaped)) < 0) || (count >= size - 1)) {
+    /* Grow buffer if necessary and retry */
+    if (count >= size) {
         buf->content[buf->use] = 0;
-        grow_size = (count > 1000) ? count : 1000;
+        grow_size = (count + 1 > 1000) ? count + 1 : 1000;
         if (virBufferGrow(buf, grow_size) < 0) {
-            VIR_FREE(escaped);
-            return;
+            goto err;
+        }
+        size = buf->size - buf->use;
+
+        if ((count = snprintf(&buf->content[buf->use], size,
+                              format, (char *)escaped)) < 0) {
+            buf->error = 1;
+            goto err;
         }
-        size = buf->size - buf->use - 1;
     }
     buf->use += count;
-    buf->content[buf->use] = '\0';
+
+err:
     VIR_FREE(escaped);
 }
 

diff --git a/tests/.gitignore b/tests/.gitignore
index 3f32939e4863ab71c2e963ca70ccb689d6aac481..e7c74c52389badc609ef5235b83c58f7d7a1f2bb 100644 (file)
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -21,6 +21,7 @@ storagepoolxml2xmltest
 nodeinfotest
 statstest
 qparamtest
+virbuftest
 seclabeltest
 eventtest
 *.exe

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1dc7f66bf80fcec74e1d46abb347f81011cf50ed..dd59034efc5b6053d06aaf49bf895349f51942b7 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -73,7 +73,7 @@ EXTRA_DIST =          \
        qemuhelpdata
 
 check_PROGRAMS = virshtest conftest \
-        nodeinfotest statstest qparamtest
+        nodeinfotest statstest qparamtest virbuftest
 
 if WITH_XEN
 check_PROGRAMS += xml2sexprtest sexpr2xmltest \
@@ -151,6 +151,7 @@ TESTS = virshtest \
         nodeinfotest \
        statstest \
        qparamtest \
+       virbuftest \
        $(test_scripts)
 
 if WITH_XEN
@@ -352,6 +353,10 @@ qparamtest_SOURCES = \
        qparamtest.c testutils.h testutils.c
 qparamtest_LDADD = $(LDADDS)
 
+virbuftest_SOURCES = \
+       virbuftest.c testutils.h testutils.c
+virbuftest_LDADD = $(LDADDS)
+
 if WITH_LIBVIRTD
 eventtest_SOURCES = \
        eventtest.c testutils.h testutils.c ../daemon/event.c

diff --git a/tests/virbuftest.c b/tests/virbuftest.c
new file mode 100644 (file)
index 0000000..d8258fc
--- /dev/null
+++ b/tests/virbuftest.c
@@ -0,0 +1,86 @@
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "internal.h"
+#include "util.h"
+#include "testutils.h"
+#include "buf.h"
+#include "memory.h"
+
+#define TEST_ERROR(...)                             \
+    do {                                            \
+        if (virTestGetDebug())                      \
+            fprintf(stderr, __VA_ARGS__);           \
+    } while (0)
+
+struct testInfo {
+    int doEscape;
+};
+
+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;
+
+    /* This relies of virBuffer internals, so may break if things change
+     * in the future */
+    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);
+        goto out;
+    }
+
+    /*
+     * Infinite loop triggers if:
+     * (strlen + 1 > 1000) && (strlen == buf-size - buf-use - 1)
+     */
+    if (virAsprintf(&addstr, "%*s", buf->a - buf->b - 1, "a") < 0) {
+        goto out;
+    }
+
+    if (info->doEscape)
+        virBufferEscapeString(buf, "%s", addstr);
+    else
+        virBufferVSprintf(buf, "%s", addstr);
+
+    ret = 0;
+out:
+    bufret = virBufferContentAndReset(buf);
+    if (!bufret) {
+        TEST_ERROR("Buffer had error set");
+        ret = -1;
+    }
+
+    VIR_FREE(addstr);
+    VIR_FREE(bufret);
+    return ret;
+}
+
+static int
+mymain(int argc ATTRIBUTE_UNUSED,
+       char **argv ATTRIBUTE_UNUSED)
+{
+    int ret = 0;
+
+
+# define DO_TEST(msg, cb, data)                                        \
+    do {                                                               \
+        struct testInfo info = { data };                               \
+        if (virtTestRun("Buf: " msg, 1, cb, &info) < 0)                 \
+            ret = -1;                                                  \
+    } while (0)
+
+    DO_TEST("EscapeString infinite loop", testBufInfiniteLoop, 1);
+    DO_TEST("VSprintf infinite loop", testBufInfiniteLoop, 0);
+
+    return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
+}
+
+VIRT_TEST_MAIN(mymain)