[Libvir] handle PyTuple_New's malloc failure

[Daniel P. Berrange]

On Thu, Jan 17, 2008 at 07:59:07PM +0100, Jim Meyering wrote:
> "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.
> 
> 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.

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  -=|