6:41 p.m.
python/libvir.c calls PyTuple_New, but doesn't handle the possibility
of a NULL return value. Subsequent use of the result pointer in e.g.,
PyTuple_SetItem, would dereference it.
This fixes most of them, but leaves the ones in
virConnectCredCallbackWrapper untouched. Fixing them
properly is not as easy, and IMHO will require actual testing.
In this patch, I combined each new test like this
((info = PyTuple_New(8)) == NULL)
with the preceding "if (c_retval < 0)", since the bodies would be the same.
Duplicating that 2-line body might look more readable,
depending on which side of the bed you get up on...
I can go either way.
Subject: [PATCH] python/libvir.c: Handle PyTuple_New's malloc failure.
---
python/libvir.c | 27 ++++++++++++---------------
1 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/python/libvir.c b/python/libvir.c
index 3b41dc1..13b809f 100644
--- a/python/libvir.c
+++ b/python/libvir.c
@@ -48,17 +48,16 @@ libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
if (!PyArg_ParseTuple(args, (char *)"Oz:virDomainBlockStats",
&pyobj_domain,&path))
- return(NULL);
+ return(NULL);
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
c_retval = virDomainBlockStats(domain, path, &stats, sizeof(stats));
- if (c_retval < 0) {
+ if (c_retval < 0 || (info = PyTuple_New(5)) == NULL) {
Py_INCREF(Py_None);
- return(Py_None);
+ return(Py_None);
}
- /* convert to a Python tupple of long objects */
- info = PyTuple_New(5);
+ /* convert to a Python tuple of long objects */
PyTuple_SetItem(info, 0, PyLong_FromLongLong(stats.rd_req));
PyTuple_SetItem(info, 1, PyLong_FromLongLong(stats.rd_bytes));
PyTuple_SetItem(info, 2, PyLong_FromLongLong(stats.wr_req));
@@ -82,13 +81,12 @@ libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
c_retval = virDomainInterfaceStats(domain, path, &stats, sizeof(stats));
- if (c_retval < 0) {
+ if (c_retval < 0 || (info = PyTuple_New(8)) == NULL) {
Py_INCREF(Py_None);
- return(Py_None);
+ return(Py_None);
}
- /* convert to a Python tupple of long objects */
- info = PyTuple_New(8);
+ /* convert to a Python tuple of long objects */
PyTuple_SetItem(info, 0, PyLong_FromLongLong(stats.rx_bytes));
PyTuple_SetItem(info, 1, PyLong_FromLongLong(stats.rx_packets));
PyTuple_SetItem(info, 2, PyLong_FromLongLong(stats.rx_errs));
@@ -114,12 +112,11 @@ libvirt_virGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args ATTRIBUT
virError err;
PyObject *info;
- if (virCopyLastError(&err) <= 0) {
+ if (virCopyLastError(&err) <= 0 || (info = PyTuple_New(9)) == NULL) {
Py_INCREF(Py_None);
- return(Py_None);
+ return(Py_None);
}
- info = PyTuple_New(9);
PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code));
PyTuple_SetItem(info, 1, PyInt_FromLong((long) err.domain));
PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err.message));
@@ -145,12 +142,12 @@ libvirt_virConnGetLastError(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
return(NULL);
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
- if (virConnCopyLastError(conn, &err) <= 0) {
+ if (virConnCopyLastError(conn, &err) <= 0
+ || (info = PyTuple_New(9)) == NULL) {
Py_INCREF(Py_None);
- return(Py_None);
+ return(Py_None);
}
- info = PyTuple_New(9);
PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code));
PyTuple_SetItem(info, 1, PyInt_FromLong((long) err.domain));
PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err.message));
--
1.5.4.rc3.14.g44397
6:52 p.m.
On Thu, Jan 17, 2008 at 04:41:49PM +0100, Jim Meyering wrote:
I think it'd be nicer to keep the PyTuple_new bit separate as it is
now, because in some APIs there can be other code between the existing
c_retval < 0 check, and the point at which we create the Tuple.
eg, the libvirt_virDomainGetSchedulerParameters method in the
patch I'm attaching which implements a number of missing APIs
NB, this patch is not tested yet, so not intended to be applied
Might be useful to have a macro to combine the Py_INCREF and return
(Py_None) in one convenient blob. return VIR_PY_NONE();
I curious as to why we ever return(NULL) here rather than Py_None. I'm
not sure of the python runtime / C binding contract - but returning an
actual NULL seems odd.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
9:59 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
Might be useful to have a macro to combine the Py_INCREF and return
(Py_None) in one convenient blob. return VIR_PY_NONE();
Good idea. That makes the patch a lot bigger, but makes the
resulting code more readable, too.
Makes sense. And with VIR_PY_NONE, the duplication isn't a problem.
New patch below.
Me too.
Here's the combined patch.
Considering the number of uses of VIR_PY_NONE this introduces,
I'll have to do the right thing and split it into two: one that
adds VIR_PY_NONE, and another that fixes the NULL-deref bugs.
10:28 p.m.
On Thu, Jan 17, 2008 at 07:59:07PM +0100, Jim Meyering wrote:
ACK, this works out rather nicely !
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
11:54 p.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
Thanks.
Due diligence: here are the two patches I expect to commit:
(in this order)
Factor out some duplication.
* python/libvir.c (VIR_PY_NONE): New macro, to encapsulate
a common two-statement sequence.
Replace all such 2-stmt sequences.
Handle PyTuple_New's malloc failure.
* python/libvir.c (libvirt_virDomainBlockStats): Handle a NULL
return from PyTuple_New.
(libvirt_virDomainInterfaceStats, libvirt_virGetLastError): Likewise.
(libvirt_virConnGetLastError): Likewise.
---
python/libvir.c | 161 ++++++++++++++++++++-----------------------------------
1 files changed, 58 insertions(+), 103 deletions(-)
diff --git a/python/libvir.c b/python/libvir.c
index 3b41dc1..3db86ad 100644
--- a/python/libvir.c
+++ b/python/libvir.c
@@ -31,6 +31,11 @@ PyObject * libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED,
PyObject
PyObject * libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args);
PyObject * libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args);
+/* The two-statement sequence "Py_INCREF(Py_None); return Py_None;"
+ is so common that we encapsulate it here. Now, each use is simply
+ return VIR_PY_NONE; */
+#define VIR_PY_NONE (Py_INCREF (Py_None), Py_None)
+
/************************************************************************
* *
* Statistics *
@@ -48,14 +53,12 @@ libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
if (!PyArg_ParseTuple(args, (char *)"Oz:virDomainBlockStats",
&pyobj_domain,&path))
- return(NULL);
+ return(NULL);
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
c_retval = virDomainBlockStats(domain, path, &stats, sizeof(stats));
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
/* convert to a Python tupple of long objects */
info = PyTuple_New(5);
@@ -82,10 +85,8 @@ libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
c_retval = virDomainInterfaceStats(domain, path, &stats, sizeof(stats));
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
/* convert to a Python tupple of long objects */
info = PyTuple_New(8);
@@ -114,10 +115,8 @@ libvirt_virGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args ATTRIBUT
virError err;
PyObject *info;
- if (virCopyLastError(&err) <= 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (virCopyLastError(&err) <= 0)
+ return VIR_PY_NONE;
info = PyTuple_New(9);
PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code));
@@ -145,10 +144,8 @@ libvirt_virConnGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args)
return(NULL);
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
- if (virConnCopyLastError(conn, &err) <= 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (virConnCopyLastError(conn, &err) <= 0)
+ return VIR_PY_NONE;
info = PyTuple_New(9);
PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code));
@@ -348,10 +345,8 @@ libvirt_virConnectOpenAuth(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
if (auth.ncredtype) {
int i;
auth.credtype = malloc(sizeof(*auth.credtype) * auth.ncredtype);
- if (auth.credtype == NULL) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (auth.credtype == NULL)
+ return VIR_PY_NONE;
for (i = 0 ; i < auth.ncredtype ; i++) {
PyObject *val;
val = PyList_GetItem(pycredtype, i);
@@ -395,10 +390,8 @@ libvirt_virGetVersion (PyObject *self ATTRIBUTE_UNUSED, PyObject
*args)
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval == -1) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (c_retval == -1)
+ return VIR_PY_NONE;
if (type == NULL)
return PyInt_FromLong (libVer);
@@ -458,10 +451,8 @@ libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED,
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virConnectListDomains(conn, &ids[0], 500);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = PyList_New(c_retval);
for (i = 0;i < c_retval;i++) {
PyList_SetItem(py_retval, i, libvirt_intWrap(ids[i]));
@@ -484,22 +475,17 @@ libvirt_virConnectListDefinedDomains(PyObject *self
ATTRIBUTE_UNUSED,
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
c_retval = virConnectNumOfDefinedDomains(conn);
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
if (c_retval) {
names = malloc(sizeof(*names) * c_retval);
- if (!names) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (!names)
+ return VIR_PY_NONE;
c_retval = virConnectListDefinedDomains(conn, names, c_retval);
if (c_retval < 0) {
free(names);
- Py_INCREF(Py_None);
- return(Py_None);
+ return VIR_PY_NONE;
}
}
py_retval = PyList_New(c_retval);
@@ -530,10 +516,8 @@ libvirt_virDomainGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virDomainGetInfo(domain, &info);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = PyList_New(5);
PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.state));
PyList_SetItem(py_retval, 1, libvirt_ulongWrap(info.maxMem));
@@ -559,10 +543,8 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virNodeGetInfo(conn, &info);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = PyList_New(8);
PyList_SetItem(py_retval, 0, libvirt_constcharPtrWrap(&info.model[0]));
PyList_SetItem(py_retval, 1, libvirt_longWrap((long) info.memory >> 10));
@@ -587,18 +569,14 @@ libvirt_virDomainGetUUID(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
return(NULL);
domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
- if (domain == NULL) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (domain == NULL)
+ return VIR_PY_NONE;
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virDomainGetUUID(domain, &uuid[0]);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = PyString_FromStringAndSize((char *) &uuid[0], VIR_UUID_BUFLEN);
return(py_retval);
@@ -617,10 +595,8 @@ libvirt_virDomainLookupByUUID(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
return(NULL);
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
- if ((uuid == NULL) || (len != VIR_UUID_BUFLEN)) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if ((uuid == NULL) || (len != VIR_UUID_BUFLEN))
+ return VIR_PY_NONE;
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virDomainLookupByUUID(conn, uuid);
@@ -664,22 +640,17 @@ libvirt_virConnectListNetworks(PyObject *self ATTRIBUTE_UNUSED,
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
c_retval = virConnectNumOfNetworks(conn);
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
-
+ if (c_retval < 0)
+ return VIR_PY_NONE;
+
if (c_retval) {
names = malloc(sizeof(*names) * c_retval);
- if (!names) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (!names)
+ return VIR_PY_NONE;
c_retval = virConnectListNetworks(conn, names, c_retval);
if (c_retval < 0) {
free(names);
- Py_INCREF(Py_None);
- return(Py_None);
+ return VIR_PY_NONE;
}
}
py_retval = PyList_New(c_retval);
@@ -711,22 +682,17 @@ libvirt_virConnectListDefinedNetworks(PyObject *self
ATTRIBUTE_UNUSED,
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
c_retval = virConnectNumOfDefinedNetworks(conn);
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
if (c_retval) {
names = malloc(sizeof(*names) * c_retval);
- if (!names) {
- Py_INCREF(Py_None);
- return (Py_None);
- }
+ if (!names)
+ return VIR_PY_NONE;
c_retval = virConnectListDefinedNetworks(conn, names, c_retval);
if (c_retval < 0) {
free(names);
- Py_INCREF(Py_None);
- return(Py_None);
+ return VIR_PY_NONE;
}
}
py_retval = PyList_New(c_retval);
@@ -755,18 +721,14 @@ libvirt_virNetworkGetUUID(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
return(NULL);
domain = (virNetworkPtr) PyvirNetwork_Get(pyobj_domain);
- if (domain == NULL) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (domain == NULL)
+ return VIR_PY_NONE;
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virNetworkGetUUID(domain, &uuid[0]);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = PyString_FromStringAndSize((char *) &uuid[0], VIR_UUID_BUFLEN);
return(py_retval);
@@ -785,10 +747,8 @@ libvirt_virNetworkLookupByUUID(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
return(NULL);
conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
- if ((uuid == NULL) || (len != VIR_UUID_BUFLEN)) {
- Py_INCREF(Py_None);
- return(Py_None);
- }
+ if ((uuid == NULL) || (len != VIR_UUID_BUFLEN))
+ return VIR_PY_NONE;
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virNetworkLookupByUUID(conn, uuid);
@@ -814,10 +774,8 @@ libvirt_virDomainGetAutostart(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
c_retval = virDomainGetAutostart(domain, &autostart);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return Py_None;
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = libvirt_intWrap(autostart);
return(py_retval);
}
@@ -839,10 +797,8 @@ libvirt_virNetworkGetAutostart(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
c_retval = virNetworkGetAutostart(network, &autostart);
LIBVIRT_END_ALLOW_THREADS;
- if (c_retval < 0) {
- Py_INCREF(Py_None);
- return Py_None;
- }
+ if (c_retval < 0)
+ return VIR_PY_NONE;
py_retval = libvirt_intWrap(autostart);
return(py_retval);
}
@@ -873,10 +829,9 @@ PyObject * libvirt_virNodeGetCellsFreeMemory(PyObject *self
ATTRIBUTE_UNUSED,
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0) {
- free(freeMems);
+ free(freeMems);
error:
- Py_INCREF(Py_None);
- return Py_None;
+ return VIR_PY_NONE;
}
py_retval = PyList_New(c_retval);
for (i = 0;i < c_retval;i++) {
--
1.5.4.rc3.14.g44397
---
python/libvir.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/python/libvir.c b/python/libvir.c
index 3db86ad..2a36fe1 100644
--- a/python/libvir.c
+++ b/python/libvir.c
@@ -60,8 +60,9 @@ libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
if (c_retval < 0)
return VIR_PY_NONE;
- /* convert to a Python tupple of long objects */
- info = PyTuple_New(5);
+ /* convert to a Python tuple of long objects */
+ if ((info = PyTuple_New(5)) == NULL)
+ return VIR_PY_NONE;
PyTuple_SetItem(info, 0, PyLong_FromLongLong(stats.rd_req));
PyTuple_SetItem(info, 1, PyLong_FromLongLong(stats.rd_bytes));
PyTuple_SetItem(info, 2, PyLong_FromLongLong(stats.wr_req));
@@ -88,8 +89,9 @@ libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
if (c_retval < 0)
return VIR_PY_NONE;
- /* convert to a Python tupple of long objects */
- info = PyTuple_New(8);
+ /* convert to a Python tuple of long objects */
+ if ((info = PyTuple_New(8)) == NULL)
+ return VIR_PY_NONE;
PyTuple_SetItem(info, 0, PyLong_FromLongLong(stats.rx_bytes));
PyTuple_SetItem(info, 1, PyLong_FromLongLong(stats.rx_packets));
PyTuple_SetItem(info, 2, PyLong_FromLongLong(stats.rx_errs));
@@ -118,7 +120,8 @@ libvirt_virGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args ATTRIBUT
if (virCopyLastError(&err) <= 0)
return VIR_PY_NONE;
- info = PyTuple_New(9);
+ if ((info = PyTuple_New(9)) == NULL)
+ return VIR_PY_NONE;
PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code));
PyTuple_SetItem(info, 1, PyInt_FromLong((long) err.domain));
PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err.message));
@@ -147,7 +150,8 @@ libvirt_virConnGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args)
if (virConnCopyLastError(conn, &err) <= 0)
return VIR_PY_NONE;
- info = PyTuple_New(9);
+ if ((info = PyTuple_New(9)) == NULL)
+ return VIR_PY_NONE;
PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code));
PyTuple_SetItem(info, 1, PyInt_FromLong((long) err.domain));
PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err.message));
--
1.5.4.rc3.14.g44397
11:56 p.m.
On Thu, Jan 17, 2008 at 09:54:12PM +0100, Jim Meyering wrote:
Yep, Go for it.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6303
days inactive
6303
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Jim Meyering