[libvirt] [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 accomodate for it. Here's a bug where we are actually hitting this issue: https://bugzilla.redhat.com/show_bug.cgi?id=602772 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/buf.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/util/buf.c b/src/util/buf.c index fc1217b..734b8f6 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -236,11 +236,11 @@ virBufferVSprintf(const virBufferPtr buf, const char *format, ...) virBufferGrow(buf, 100) < 0) return; - 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, - locarg)) < 0) || (count >= size - 1)) { + locarg)) < 0) || (count >= size)) { buf->content[buf->use] = 0; va_end(locarg); @@ -250,13 +250,12 @@ virBufferVSprintf(const virBufferPtr buf, const char *format, ...) return; } - 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'; } /** @@ -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) { VIR_FREE(escaped); return; } - size = buf->size - buf->use - 1; + size = buf->size - buf->use; } buf->use += count; - buf->content[buf->use] = '\0'; VIR_FREE(escaped); } -- 1.7.2.1
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). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/01/2010 04:38 PM, Eric Blake wrote:
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).
THanks for the review (online and offline). I've sent an updated patch with your recommended improvements. Thanks, Cole
participants (2)
-
Cole Robinson -
Eric Blake