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

[Martin von Zweigbergk]

On Fri Jan 23 2015 at 1:42:15 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.
>
>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150123/ee132445/attachment.html>