Re: [python PATCH] Check return value of libvirt_uintUnwrap

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

 



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@xxxxxxxxxx>
>> ---
>>
>> 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]