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

Re: [Libvir] handle PyTuple_New's malloc failure



"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.



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