Thanks for the review, Rich. Seeing that messed-up Subject: line, I realized that I sent a patch produced by "git-format-patch", forgetting that the ancient version of mailman behind this list would remove important parts of it. For the record, here's what I really sent: (with the "From " line escaped) From: Jim Meyering <jim@meyering.net> To: "Daniel P. Berrange" <berrange@redhat.com> Cc: Libvirt <libvir-list@redhat.com> Subject: Re: [Libvir] handle PyTuple_New's malloc failure In-Reply-To: <20080117155200.GC21549@redhat.com> (Daniel P. Berrange's message of "Thu, 17 Jan 2008 15:52:00 +0000") References: <87zlv4larm.fsf@rho.meyering.net> <20080117155200.GC21549@redhat.com> Date: Thu, 17 Jan 2008 19:59:07 +0100 Message-ID: <87y7aojn2c.fsf@rho.meyering.net> "Daniel P. Berrange" <berrange@redhat.com> wrote:

On Thu, Jan 17, 2008 at 04:41:49PM +0100, Jim Meyering wrote:

...

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.

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.

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

Makes sense. And with VIR_PY_NONE, the duplication isn't a problem. New patch below.

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.

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.

From 55bafd1bdaf6dde1bd019397f569d397fea366a6 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 17 Jan 2008 16:40:17 +0100 Subject: [PATCH] python/libvir.c (VIR_PY_NONE): New macro, to encapsulate a common two-statement sequence. Handle PyTuple_New's malloc failure. Use VIR_PY_NONE.

Signed-off-by: Jim Meyering <meyering@redhat.com> --- python/libvir.c | 177 +++++++++++++++++++++---------------------------------- 1 files changed, 68 insertions(+), 109 deletions(-) diff --git a/python/libvir.c b/python/libvir.c index 3b41dc1..dbf148e 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,17 +53,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) { - 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); + /* 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)); @@ -82,13 +86,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) { - 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); + /* 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)); @@ -114,12 +117,11 @@ 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); + 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)); @@ -145,12 +147,11 @@ 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); + 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)); @@ -348,10 +349,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 +394,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 +455,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 +479,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 +520,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 +547,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 +573,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 +599,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 +644,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 +686,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 +725,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 +751,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 +778,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 +801,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 +833,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