Patchwork [3,of,7] parsers: avoid leaking obj in index_ancestors

login
register
mail settings
Submitter Augie Fackler
Date Jan. 23, 2015, 9:06 p.m.
Message ID <72365a74930dfeae8b7e.1422047200@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/7542/
State Accepted
Commit ec28f8b66e622d9cbea7c7c2b1cb01842661af31
Headers show

Comments

Augie Fackler - Jan. 23, 2015, 9:06 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1422045207 18000
#      Fri Jan 23 15:33:27 2015 -0500
# Branch stable
# Node ID 72365a74930dfeae8b7e7bce3e2beef6d6d95c5f
# Parent  5de7357d24e4e5444198e82ab710dbffb9453d0a
parsers: avoid leaking obj in index_ancestors

PySequence_GetItem returns a new reference. Found with cpychecker.
Martin von Zweigbergk - Jan. 23, 2015, 9:42 p.m.
On Fri Jan 23 2015 at 1:10:58 PM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1422045207 18000
> #      Fri Jan 23 15:33:27 2015 -0500
> # Branch stable
> # Node ID 72365a74930dfeae8b7e7bce3e2beef6d6d95c5f
> # Parent  5de7357d24e4e5444198e82ab710dbffb9453d0a
> parsers: avoid leaking obj in index_ancestors
>
> PySequence_GetItem returns a new reference. Found with cpychecker.
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1691,9 +1691,11 @@ static PyObject *index_ancestors(indexOb
>                 if (!PyInt_Check(obj)) {
>                         PyErr_SetString(PyExc_TypeError,
>                                         "arguments must all be ints");
> +                       Py_DECREF(obj);
>

Nit: Could move this call to beginning of block since 'obj' is not used.


>                         goto bail;
>                 }
>                 val = PyInt_AsLong(obj);
> +               Py_DECREF(obj);
>                 if (val == -1) {
>                         ret = PyList_New(0);
>                         goto done;
>

Unrelated to this patch, but do you know why -1 results in a successful
return of an empty list? -1 from PyInt_AsLong means that an error _might_
have happened and one should check PyErr_Occurred() to see whether it
actually did. We don't check that here. Perhaps its just very unlikely
something did go wrong, but even so, why would we return an empty list when
val == -1 rather than falling through and raising a PyExc_IndexError? Maybe
-1 was supposed to mean something special? I can't make any sense of that
from what the function is supposed to return. Also, all tests pass after
removing the block. Maybe I'll just send a separate patch removing it.
Augie Fackler - Jan. 23, 2015, 9:54 p.m.
On Jan 23, 2015, at 4:42 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:

> 
> 
> On Fri Jan 23 2015 at 1:10:58 PM Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1422045207 18000
> #      Fri Jan 23 15:33:27 2015 -0500
> # Branch stable
> # Node ID 72365a74930dfeae8b7e7bce3e2beef6d6d95c5f
> # Parent  5de7357d24e4e5444198e82ab710dbffb9453d0a
> parsers: avoid leaking obj in index_ancestors
> 
> PySequence_GetItem returns a new reference. Found with cpychecker.
> 
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1691,9 +1691,11 @@ static PyObject *index_ancestors(indexOb
>                 if (!PyInt_Check(obj)) {
>                         PyErr_SetString(PyExc_TypeError,
>                                         "arguments must all be ints");
> +                       Py_DECREF(obj);
> 
> Nit: Could move this call to beginning of block since 'obj' is not used.
>  
>                         goto bail;
>                 }
>                 val = PyInt_AsLong(obj);
> +               Py_DECREF(obj);
>                 if (val == -1) {
>                         ret = PyList_New(0);
>                         goto done;
> 
> Unrelated to this patch, but do you know why -1 results in a successful return of an empty list? -1 from PyInt_AsLong means that an error _might_ have happened and one should check PyErr_Occurred() to see whether it actually did. We don't check that here. Perhaps its just very unlikely something did go wrong, but even so, why would we return an empty list when val == -1 rather than falling through and raising a PyExc_IndexError? Maybe -1 was supposed to mean something special? I can't make any sense of that from what the function is supposed to return. Also, all tests pass after removing the block. Maybe I'll just send a separate patch removing it.

I didn't meditate closely - I was too focused on working through the memory leaks. Good find.

> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Jan. 23, 2015, 9:57 p.m.
On Fri Jan 23 2015 at 1:42:15 PM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Fri Jan 23 2015 at 1:10:58 PM Augie Fackler <raf@durin42.com> wrote:
>
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1422045207 18000
>> #      Fri Jan 23 15:33:27 2015 -0500
>> # Branch stable
>> # Node ID 72365a74930dfeae8b7e7bce3e2beef6d6d95c5f
>> # Parent  5de7357d24e4e5444198e82ab710dbffb9453d0a
>> parsers: avoid leaking obj in index_ancestors
>>
>> PySequence_GetItem returns a new reference. Found with cpychecker.
>>
>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>> --- a/mercurial/parsers.c
>> +++ b/mercurial/parsers.c
>> @@ -1691,9 +1691,11 @@ static PyObject *index_ancestors(indexOb
>>                 if (!PyInt_Check(obj)) {
>>                         PyErr_SetString(PyExc_TypeError,
>>                                         "arguments must all be ints");
>> +                       Py_DECREF(obj);
>>
>
> Nit: Could move this call to beginning of block since 'obj' is not used.
>
>
>>                         goto bail;
>>                 }
>>                 val = PyInt_AsLong(obj);
>> +               Py_DECREF(obj);
>>                 if (val == -1) {
>>                         ret = PyList_New(0);
>>                         goto done;
>>
>
> Unrelated to this patch, but do you know why -1 results in a successful
> return of an empty list? -1 from PyInt_AsLong means that an error _might_
> have happened and one should check PyErr_Occurred() to see whether it
> actually did. We don't check that here. Perhaps its just very unlikely
> something did go wrong, but even so, why would we return an empty list when
> val == -1 rather than falling through and raising a PyExc_IndexError? Maybe
> -1 was supposed to mean something special? I can't make any sense of that
> from what the function is supposed to return. Also, all tests pass after
> removing the block. Maybe I'll just send a separate patch removing it.
>
>
Now I see what this is about. -1 is the null/root revision. I guess I would
have expected an singleton list containing -1 in that case. Hmm... forget
my question for now.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1691,9 +1691,11 @@  static PyObject *index_ancestors(indexOb
 		if (!PyInt_Check(obj)) {
 			PyErr_SetString(PyExc_TypeError,
 					"arguments must all be ints");
+			Py_DECREF(obj);
 			goto bail;
 		}
 		val = PyInt_AsLong(obj);
+		Py_DECREF(obj);
 		if (val == -1) {
 			ret = PyList_New(0);
 			goto done;