[libvirt] [python PATCH] Check return value of libvirt_uintUnwrap
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... libvirt-override.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libvirt-override.c b/libvirt-override.c index a53b46f..f496c6e 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -7119,47 +7119,48 @@ static PyObject * libvirt_virDomainSendKey(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { PyObject *py_retval; virDomainPtr domain; PyObject *pyobj_domain; PyObject *pyobj_list; int codeset; int holdtime; unsigned int flags; int ret; size_t i; unsigned int keycodes[VIR_DOMAIN_SEND_KEY_MAX_KEYS]; unsigned int nkeycodes; 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; } domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); if (!PyList_Check(pyobj_list)) { return VIR_PY_INT_FAIL; } if (nkeycodes != PyList_Size(pyobj_list) || nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) { return VIR_PY_INT_FAIL; } 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; } LIBVIRT_BEGIN_ALLOW_THREADS; ret = virDomainSendKey(domain, codeset, holdtime, keycodes, nkeycodes, flags); LIBVIRT_END_ALLOW_THREADS; DEBUG("virDomainSendKey ret=%d\n", ret); py_retval = libvirt_intWrap(ret); return py_retval; } #if LIBVIR_CHECK_VERSION(1, 0, 3) -- 2.1.3
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...
libvirt-override.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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...
libvirt-override.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libvirt-override.c b/libvirt-override.c index a53b46f..f496c6e 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -7119,47 +7119,48 @@ static PyObject * libvirt_virDomainSendKey(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { PyObject *py_retval; virDomainPtr domain; PyObject *pyobj_domain; PyObject *pyobj_list; int codeset; int holdtime; unsigned int flags; int ret; size_t i; unsigned int keycodes[VIR_DOMAIN_SEND_KEY_MAX_KEYS]; unsigned int nkeycodes;
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; } domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
if (!PyList_Check(pyobj_list)) { return VIR_PY_INT_FAIL; }
if (nkeycodes != PyList_Size(pyobj_list) || nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) { return VIR_PY_INT_FAIL; }
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, Pavel
}
LIBVIRT_BEGIN_ALLOW_THREADS; ret = virDomainSendKey(domain, codeset, holdtime, keycodes, nkeycodes, flags); LIBVIRT_END_ALLOW_THREADS;
DEBUG("virDomainSendKey ret=%d\n", ret);
py_retval = libvirt_intWrap(ret); return py_retval; }
#if LIBVIR_CHECK_VERSION(1, 0, 3)
On Thu, Nov 06, 2014 at 12:04:54 +0100, 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...
libvirt-override.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libvirt-override.c b/libvirt-override.c index a53b46f..f496c6e 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -7119,47 +7119,48 @@ static PyObject * libvirt_virDomainSendKey(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { PyObject *py_retval; virDomainPtr domain; PyObject *pyobj_domain; PyObject *pyobj_list; int codeset; int holdtime; unsigned int flags; int ret; size_t i; unsigned int keycodes[VIR_DOMAIN_SEND_KEY_MAX_KEYS]; unsigned int nkeycodes;
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; } domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
if (!PyList_Check(pyobj_list)) { return VIR_PY_INT_FAIL; }
if (nkeycodes != PyList_Size(pyobj_list) || nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) { return VIR_PY_INT_FAIL; }
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,
Fixed and pushed, thanks. Jirka
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
participants (3)
-
Eric Blake -
Jiri Denemark -
Pavel Hrdina