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, ...) > return; > } > 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); > } > va_end(argptr); > va_end(locarg); > 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 > return; > } > > - 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).