From 18af6f4e64e97095bc95df25fb4a092cbbd6474c Mon Sep 17 00:00:00 2001 From: Cole Robinson Date: Wed, 1 Sep 2010 13:51:35 -0400 Subject: [PATCH] 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 --- src/util/buf.c | 72 ++++++++++++++++++++++++-------------- tests/.gitignore | 1 + tests/Makefile.am | 7 +++- tests/virbuftest.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 26 deletions(-) create mode 100644 tests/virbuftest.c diff --git a/src/util/buf.c b/src/util/buf.c index fc1217bf60..553e2a059e 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)) { - buf->content[buf->use] = 0; - va_end(locarg); - grow_size = (count > 1000) ? count : 1000; + 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(argptr); + va_start(argptr, format); + + 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 - 1; - while (((count = snprintf(&buf->content[buf->use], size, format, - (char *)escaped)) < 0) || (count >= size - 1)) { + size = buf->size - buf->use; + if ((count = snprintf(&buf->content[buf->use], size, + format, (char *)escaped)) < 0) { + buf->error = 1; + goto err; + } + + /* 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 3f32939e48..e7c74c5238 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 1dc7f66bf8..dd59034efc 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 0000000000..d8258fcaff --- /dev/null +++ b/tests/virbuftest.c @@ -0,0 +1,86 @@ +#include + +#include +#include +#include + +#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)