| Submitter | Bryan O'Sullivan |
|---|---|
| Date | Dec. 13, 2015, 4:11 a.m. |
| Message ID | <6c0dd7cd82797705c5be.1449979910@bryano-mbp.netgear.com> |
| Download | mbox | patch |
| Permalink | /patch/11997/ |
| State | Superseded |
| Delegated to: | Yuya Nishihara |
| Headers | show |
Comments
On Sat, 12 Dec 2015 20:11:50 -0800, Bryan O'Sullivan wrote: > # HG changeset patch > # User Bryan O'Sullivan <bos@serpentine.com> > # Date 1449979901 28800 > # Sat Dec 12 20:11:41 2015 -0800 > # Node ID 6c0dd7cd82797705c5be51be892afd94a4909a65 > # Parent 18f300e9ca45cdae3575d1860251929abf07b42c > parsers: simplify error logic in compute_phases_map_sets > > Since Py_XDECREF and free both accept NULL pointers, we can get by > with just two exit paths: one for success, and one for error. > > This considerably simplifies reasoning about the possible ways to > exit from this function. and fixes memleak if phasessetlist == NULL. > diff --git a/mercurial/parsers.c b/mercurial/parsers.c > --- a/mercurial/parsers.c > +++ b/mercurial/parsers.c > @@ -1281,19 +1281,19 @@ static PyObject *compute_phases_map_sets > long phase; > > if (!PyArg_ParseTuple(args, "O", &roots)) > - goto release_none; > + goto done; > if (roots == NULL || !PyList_Check(roots)) > - goto release_none; > + goto done; > phases = calloc(len, 1); /* phase per rev: {0: public, 1: draft, 2: secret} */ > if (phases == NULL) > - goto release_none; > + goto done; Oops, I found PyErr_NoMemory() was missing here. > /* Put the phase information of all the roots in phases */ > numphase = PyList_GET_SIZE(roots)+1; > minrevallphases = len + 1; > phasessetlist = PyList_New(numphase); > if (phasessetlist == NULL) > - goto release_none; > + goto done; From your comment, "one for success, one for error", I think these "goto done" should be "goto release" even though there are no difference. > ret = PyList_New(2); > if (ret == NULL) > - goto release_phaseslist; > + goto release; > > PyList_SET_ITEM(ret, 0, phaseslist); > PyList_SET_ITEM(ret, 1, phasessetlist); > /* We don't release phaseslist and phasessetlist as we return them to > * python */ > - goto release_phases; > + goto done; This is off topic, but perhaps it can be written by PyTuple_Pack(). > -release_phaseslist: > +release: > Py_XDECREF(phaseslist); > -release_phasesetlist: > Py_XDECREF(phasessetlist); > -release_phases: > +done: > free(phases); > -release_none: > return ret; > }
On Sat, Dec 12, 2015 at 11:30 PM, Yuya Nishihara <yuya@tcha.org> wrote: > Oops, I found PyErr_NoMemory() was missing here. > I'll follow up with a revised patch.
Patch
diff --git a/mercurial/parsers.c b/mercurial/parsers.c --- a/mercurial/parsers.c +++ b/mercurial/parsers.c @@ -1281,19 +1281,19 @@ static PyObject *compute_phases_map_sets long phase; if (!PyArg_ParseTuple(args, "O", &roots)) - goto release_none; + goto done; if (roots == NULL || !PyList_Check(roots)) - goto release_none; + goto done; phases = calloc(len, 1); /* phase per rev: {0: public, 1: draft, 2: secret} */ if (phases == NULL) - goto release_none; + goto done; /* Put the phase information of all the roots in phases */ numphase = PyList_GET_SIZE(roots)+1; minrevallphases = len + 1; phasessetlist = PyList_New(numphase); if (phasessetlist == NULL) - goto release_none; + goto done; PyList_SET_ITEM(phasessetlist, 0, Py_None); Py_INCREF(Py_None); @@ -1302,13 +1302,13 @@ static PyObject *compute_phases_map_sets phaseroots = PyList_GET_ITEM(roots, i); phaseset = PySet_New(NULL); if (phaseset == NULL) - goto release_phasesetlist; + goto release; PyList_SET_ITEM(phasessetlist, i+1, phaseset); if (!PyList_Check(phaseroots)) - goto release_phasesetlist; + goto release; minrevphase = add_roots_get_min(self, phaseroots, i+1, phases); if (minrevphase == -2) /* Error from add_roots_get_min */ - goto release_phasesetlist; + goto release; minrevallphases = MIN(minrevallphases, minrevphase); } /* Propagate the phase information from the roots to the revs */ @@ -1317,14 +1317,14 @@ static PyObject *compute_phases_map_sets for (i = minrevallphases; i < len; i++) { if (index_get_parents(self, i, parents, (int)len - 1) < 0) - goto release_phasesetlist; + goto release; set_phase_from_parents(phases, parents[0], parents[1], i); } } /* Transform phase list to a python list */ phaseslist = PyList_New(len); if (phaseslist == NULL) - goto release_phasesetlist; + goto release; for (i = 0; i < len; i++) { phase = phases[i]; /* We only store the sets of phase for non public phase, the public phase @@ -1339,21 +1339,19 @@ static PyObject *compute_phases_map_sets } ret = PyList_New(2); if (ret == NULL) - goto release_phaseslist; + goto release; PyList_SET_ITEM(ret, 0, phaseslist); PyList_SET_ITEM(ret, 1, phasessetlist); /* We don't release phaseslist and phasessetlist as we return them to * python */ - goto release_phases; + goto done; -release_phaseslist: +release: Py_XDECREF(phaseslist); -release_phasesetlist: Py_XDECREF(phasessetlist); -release_phases: +done: free(phases); -release_none: return ret; }