[libvirt] [PATCH v2] 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 v2: Eric's improvements: while -> if (), remove extra va_list variable, make sure we report buffer error if snprintf fails Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/buf.c | 70 ++++++++++++++++++++++++++++++++++++------------------- 1 files changed, 46 insertions(+), 24 deletions(-) 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); } -- 1.7.2.1
On 09/01/2010 03:41 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
s/accomodate/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
ACK, with one spelling nit in the commit message. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/01/2010 03:41 PM, Cole Robinson wrote:
+ size = buf->size - buf->use; + if ((count = vsnprintf(&buf->content[buf->use], + size, format, argptr))< 0) { + buf->error = 1; + goto err; + }
Hmm, thinking about this a bit more, most callers blindly assume that if virBufferContentAndReset returns NULL, then they should report OOM. But in this case, OOM is a bit misleading if the real cause is that the format string was invalid and snprintf died with EINVAL (OOM implies good code but insufficient runtime resources; while EINVAL implies a programming error that should immediately be reported as a bug and fixed). Don't make any changes to your patch (it's fine as is: it fixes a real bug, and we are unlikely to hit the misleading OOM message since we shouldn't be passing bad format strings in the first place). But I'm wondering if a future patch should change buf->error to track the actual errno value of the first failure encountered (ENOMEM for the common case, but EILSEQ, EINVAL, EOVERFLOW for snprintf failures), and add a function like virBufferReportError() which calls the appropriate error message (virReportOOMError() vs. xxx(VIR_ERR_INTERNAL_ERROR, errno, "unexpected failure"). However, this would mean an audit of all existing virBuffer clients to call virBufferReportError() instead of virReportOOMError() on failure. Or indeed, if _all_ callers already (should) print errors, maybe it makes sense to inline the OOM error printing into virBufferContentAndReset in the first place. [I thought of this because the pending virCommand patch series has the same issue - not all failures are ENOMEM related, and it would be nice for virCommandRun to distinguish between the failures. But with the current state of those patches, the contract is that virCommandRun prints the error, rather than leaving it up to the caller.] -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Sep 01, 2010 at 05:41:46PM -0400, 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.
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
How about adding a unit test for the virBuffer APIs to verify all this stuff is working as designed. It is nicely self-contained code so we ought to be able to get 100% coverage of all codepaths and error conditions like this one Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 09/02/2010 04:47 AM, Daniel P. Berrange wrote:
On Wed, Sep 01, 2010 at 05:41:46PM -0400, 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.
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
How about adding a unit test for the virBuffer APIs to verify all this stuff is working as designed. It is nicely self-contained code so we ought to be able to get 100% coverage of all codepaths and error conditions like this one
Daniel
I sent an updated patch with a unittest that reproduces the infinite loop as a start. Thanks, Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake