On Thu, Jan 17, 2008 at 07:59:07PM +0100, Jim Meyering wrote: > "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 -=| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list