"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. -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list