[PATCH 6 of 6 cpychecker] parsers: avoid over-retaining Py_None if index_init fails

Augie Fackler raf at durin42.com
Fri Aug 21 12:07:06 CDT 2015


On Fri, Aug 21, 2015 at 12:26:44AM +0900, Yuya Nishihara wrote:
> On Tue, 18 Aug 2015 17:54:16 -0400, Augie Fackler wrote:
> > # HG changeset patch
> > # User Augie Fackler <augie at google.com>
> > # Date 1439931329 14400
> > #      Tue Aug 18 16:55:29 2015 -0400
> > # Node ID 87c8ccbc26ccaddf628f2d09323d4664cc8066eb
> > # Parent  b9c2bab361f6abe80f7a155ba7910cf0a6bd90d8
> > parsers: avoid over-retaining Py_None if index_init fails
> >
> > Spotted with cpychecker. Note that cpychecker still thinks there's a
> > problem with over-retaining Py_None here, but I'm pretty sure it's
> > wrong by inspection.
> >
> > I also noticed that self->data could end up over-retained, so I fixed
> > that as well. cpychecker is silent both before and after that change,
> > and I'm pretty sure it's right.
> >
> > diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> > --- a/mercurial/parsers.c
> > +++ b/mercurial/parsers.c
> > @@ -2349,15 +2349,15 @@ static int index_init(indexObject *self,
> >     self->data = NULL;
> >     self->headrevs = NULL;
> >     self->filteredrevs = Py_None;
> > -	Py_INCREF(Py_None);
> > +	Py_INCREF(self->filteredrevs);
> >     self->nt = NULL;
> >     self->offsets = NULL;
> >
> >     if (!PyArg_ParseTuple(args, "OO", &data_obj, &inlined_obj))
> > -		return -1;
> > +		goto bail;
> >     if (!PyString_Check(data_obj)) {
> >             PyErr_SetString(PyExc_TypeError, "data is not a string");
> > -		return -1;
> > +		goto bail;
> >     }
> >     size = PyString_GET_SIZE(data_obj);
> >
> > @@ -2387,6 +2387,8 @@ static int index_init(indexObject *self,
> >
> >     return 0;
> >  bail:
> > +	Py_DECREF(self->filteredrevs);
> > +	Py_XDECREF(self->data);
>
> Perhaps this doubles decrefs because the index object should exist no matter
> if index_init() fail and index_dealloc() would be called anyway?

My *guess* is that if tp_init fails then tp_dealloc is never called. I
can't find any documentation one way or the other though, so I'm happy
to let this one linger.

I'll queue patches 1-5 per your review, thanks

>
> The other patches look good to me, but I'm newcomer to C API.


More information about the Mercurial-devel mailing list