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