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

[Yuya Nishihara]

On Fri, 21 Aug 2015 13:07:06 -0400, Augie Fackler wrote:
> 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.

If tp_init() fails, self is Py_DECREF()-ed. So I thought it should call
tp_dealloc().

https://hg.python.org/cpython/file/v2.7.10/Objects/typeobject.c#l743