[Libvir] handle PyTuple_New's malloc failure
jim at meyering.net
Thu Jan 17 18:59:07 UTC 2008
"Daniel P. Berrange" <berrange at 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.
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.
More information about the libvir-list