[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [python PATCH] Check return value of libvirt_uintUnwrap



On 11/06/2014 12:04 PM, Pavel Hrdina wrote:
> On 11/06/2014 11:05 AM, Jiri Denemark wrote:
>> libvirt_virDomainSendKey didn't check whether libvirt_uintUnwrap
>> succeeded or not.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1161039
>> Signed-off-by: Jiri Denemark <jdenemar redhat com>
>> ---
>>
>> Providing extra context for easier review...
>>

>>
>>       if (!PyArg_ParseTuple(args, (char *)"OiiOiI:virDomainSendKey",
>>                             &pyobj_domain, &codeset, &holdtime,
>> &pyobj_list,
>>                             &nkeycodes, &flags)) {
>>           DEBUG("%s failed to parse tuple\n", __FUNCTION__);
>>           return VIR_PY_INT_FAIL;

Pavel makes a good point.  Returning VIR_PY_INT_FAIL (aka -1) implies
that we have set a stack-local libvirt error to be turned into a libvirt
exception.  Returning NULL implies that we do not have a libvirt error,
but DO have a python error.

This existing code is wrong and should return NULL here.

A quick audit of libvirt-override.c found that at least
libvirt_virDomainGetJobStats(), libvirt_virConnectDomainEventRegister(),
libvirt_virEventRegisterImpl(), libvirt_virEventInvokeHandleCallback(),
and others also have a bug with this.

>>       }
>>       domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>>
>>       if (!PyList_Check(pyobj_list)) {
>>           return VIR_PY_INT_FAIL;
>>       }

This code is wrong and should return NULL.

>>
>>       if (nkeycodes != PyList_Size(pyobj_list) ||
>>           nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) {
>>           return VIR_PY_INT_FAIL;
>>       }

This code is wrong - it needs to raise a python error, then return NULL.

>>
>>       for (i = 0; i < nkeycodes; i++) {
>> -        libvirt_uintUnwrap(PyList_GetItem(pyobj_list, i),
>> &(keycodes[i]));
>> +        if (libvirt_uintUnwrap(PyList_GetItem(pyobj_list, i),
>> &keycodes[i]) < 0)
>> +            return VIR_PY_INT_FAIL;
> 
> Return NULL instead of PyObject with value -1. Function
> "libvirt_uintUnwrap" sets an exception on error and in that case the
> NULL should be returned.
> 
> ACK with that change,

As the mis-use of -1 instead of NULL is more pervasive than just this
function, we need a cleanup patch to cover all the wrong uses.

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]