On 09/09/2010 10:50 PM, Eric Blake wrote: > On 09/09/2010 08:09 AM, 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 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 > > I confirm that the only change in v3 is the addition of a test. > >> +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); > > This seems a bit fragile; it is inspecting the innards of virBuffer. > Maybe add a comment here that any change to the innards of virBuffer may > warrant a change to this test at the same time. But having the test, > even if it is fragile, is a good thing, so... > > ACK. >
Thanks, pushed with a comment. - Cole > We could add further tests to this file to cover more lines of code, > such as: printing a string that needs escapes added, and actually > verifying that the escaped string matches expectations; passing a bad > format string (such as "%$2s" to trigger EINVAL, > "%*d%*d",1<<31,1,1<<31,1 to trigger EOVERFLOW) and verifying that error > gets set. But those can be in a separate patch; let's get the bug fix > in now before 0.8.4. > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
