9:09 a.m.
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
---
src/util/buf.c | 70 ++++++++++++++++++++++++++++---------------
tests/.gitignore | 1 +
tests/Makefile.am | 7 ++++-
tests/virbuftest.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 137 insertions(+), 25 deletions(-)
create mode 100644 tests/virbuftest.c
diff --git a/src/util/buf.c b/src/util/buf.c
index fc1217b..553e2a0 100644
--- 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 3f32939..e7c74c5 100644
--- 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 1dc7f66..dd59034 100644
--- 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
index 0000000..1cc9410
--- /dev/null
+++ b/tests/virbuftest.c
@@ -0,0 +1,84 @@
+#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;
+
+ 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)
--
1.7.2.1