| 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
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.
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
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;