[libvirt] [PATCH 05/24] maint: don't leave garbage on early API exit

[Eric Blake]

On 01/02/2014 11:24 AM, John Ferlan wrote:
> 
> 
> On 12/28/2013 11:11 AM, Eric Blake wrote:
>> Several APIs clear out a user input buffer before attempting to
>> populate it; but in a few cases we missed this memset if we
>> detect a reason for an early exit.
>>
>> * src/libvirt.c (virDomainGetInfo, virDomainGetBlockInfo)
>> (virStoragePoolGetInfo, virStorageVolGetInfo)
>> (virDomainGetJobInfo, virDomainGetBlockJobInfo): Move memset
>> before any returns.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---
>>  src/libvirt.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
> 
> Not sure any of these are correct... Each of the clear outs is after a
> check of whether the passed 'info' buffer is non NULL.
> 
> For each that means you'd potentially have "memset(NULL, 0, sizeof())"

Ooh, you make a good point.  The caller has a bug in that case, and we
could technically argue that they deserve the segfault (passing a bad
pointer is the classic case of undefined behavior, and faulting will
call attention to their bad code faster than an error code that might be
ignored).  But just because they deserve it doesn't mean we should do
it, especially since it is a change in behavior for sloppy programs that
currently get a nice error message but would now crash (we can't detect
every single bad pointer, but NULL is a special case that is both more
likely than any other bad pointer, and also easy enough to single out).

So on the grounds of accommodating existing sloppy use, I'm thinking
that it's best to change all of these to make the memset conditional on
a non-NULL pointer, but still moving the memset to occur before other
early exits (that is, we guarantee that if the buffer is valid, we clear
it; and if the buffer is NULL, we guarantee a sane error message).  My
rework of this patch gets delayed to v2.


By the way, the motivation behind this patch is functions like
virConnectListAllDomains, which takes pains to ensure *domains is set to
NULL regardless of error, but which DID play it safe:

virConnectListAllDomains(virConnectPtr conn,
                         virDomainPtr **domains,
                         unsigned int flags)
{
    VIR_DEBUG("conn=%p, domains=%p, flags=%x", conn, domains, flags);

    virResetLastError();

    if (domains)
        *domains = NULL;

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140103/2acbc129/attachment-0001.sig>