[libvirt] [PATCH v2] buf: Fix possible infinite loop in EscapeString, VSnprintf

Eric Blake eblake at redhat.com
Wed Sep 1 22:18:13 UTC 2010


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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list