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

login
register
mail settings
Submitter Augie Fackler
Date Aug. 18, 2015, 9:54 p.m.
Message ID <87c8ccbc26ccaddf628f.1439934856@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/10239/
State Rejected
Delegated to: Augie Fackler
Headers show

Comments

Augie Fackler - Aug. 18, 2015, 9:54 p.m.
# HG changeset patch
# User Augie Fackler <augie@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.
Yuya Nishihara - Aug. 20, 2015, 3:26 p.m.
On Tue, 18 Aug 2015 17:54:16 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@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?

The other patches look good to me, but I'm newcomer to C API.
Augie Fackler - Aug. 21, 2015, 5:07 p.m.
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@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.
Yuya Nishihara - Aug. 22, 2015, 2:40 a.m.
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@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

Patch

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);
 	return -1;
 }