[libvirt] [PATCH 2/2] Protect against NULL pointer flaws in monitor usage

[Eric Blake]

On 05/17/2010 08:54 AM, Daniel P. Berrange wrote:
>>>      int ret;
>>> -    DEBUG("mon=%p, fd=%d", mon, mon->fd);
>>> +    DEBUG("mon=%p", mon);
>>> +
>>> +    if (!mon) {
>>> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                        _("monitor must not be NULL"));
>>> +        return -1;
>>> +    }
>>
>> Wouldn't it be better to move the DEBUG() to be after the (!mon) check,
>> so that we can still print mon->fd?  (Throughout the patch).
> 
> Yes & no. In practice I've never found a need to look at the 'fd'
> parameter being logged. So I think it is more important to log
> every call, so that we can see cases there 'mon=(null)' instead of
> them being skipped.

Fair enough; besides, the fact that mon is logged, a gdb session can
easily get at mon->fd from the logged pointer.

>> Likewise.  And while it was nice to add password=%p,...
>>
>>>  
>>>      if (!password)
>>>          password = "";
>>
>> ...you may have just dereferenced another NULL pointer (at least DEBUG
>> tends to only be used with glibc, where you get a sane "(null)" instead
>> of a crash).
> 
> Where is the NULL de-reference ? We're using %p, not %s for logging the
> password because we don't want to actually expose the password string
> in the logs & %p doesn't de-reference the pointer.

Chalk it up to an early morning on my part.  You're right, that %p is
safe on NULL.  So, given your rebuttals, none of my points remain, and
you now have my:

ACK.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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