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

[Augie Fackler]

On Jan 23, 2015, at 4:42 PM, Martin von Zweigbergk <martinvonz at google.com> wrote:

> 
> 
> On Fri Jan 23 2015 at 1:10:58 PM Augie Fackler <raf at durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie at 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 at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150123/f74f5172/attachment.pgp>