| Submitter | Augie Fackler |
|---|---|
| Date | Jan. 5, 2016, 3:57 p.m. |
| Message ID | <700ae5816d662e0e5dde.1452009479@imladris.local> |
| Download | mbox | patch |
| Permalink | /patch/12532/ |
| State | Superseded |
| Commit | abc79f44f54812c7c6a434f32e4f076908ef2daa |
| Delegated to: | Martin von Zweigbergk |
| Headers | show |
Comments
On Tue, Jan 5, 2016 at 8:01 AM Augie Fackler <raf@durin42.com> wrote: > # HG changeset patch > # User Augie Fackler <raf@durin42.com> > # Date 1451586702 18000 > # Thu Dec 31 13:31:42 2015 -0500 > # Node ID 700ae5816d662e0e5dde55c68944329b6eb1b7c0 > # Parent 0bc71f45d3623b231cc3975b48feccce79d1231e > lazymanifest: check more return values in filtercopy > > Spotted by Bryan O'Sullivan (and vexingly not the static analyzer I've > been using.) > > diff --git a/mercurial/manifest.c b/mercurial/manifest.c > --- a/mercurial/manifest.c > +++ b/mercurial/manifest.c > @@ -707,11 +707,20 @@ static lazymanifest *lazymanifest_filter > copy->pydata = self->pydata; > Py_INCREF(self->pydata); > for (i = 0; i < self->numlines; i++) { > - PyObject *arg = PyString_FromString(self->lines[i].start); > - PyObject *arglist = PyTuple_Pack(1, arg); > - PyObject *result = PyObject_CallObject(matchfn, arglist); > + PyObject *arg = NULL, *arglist = NULL, *result = NULL; > + arg = PyString_FromString(self->lines[i].start); > + if (!arg) { > + PyErr_SetString(PyExc_TypeError, > + "couldn't pack filename"); > TypeError seems wrong here. Most likely out of memory? Should we just leave it as whatever it was already set to? We don't set it when PyTuple_Pack() below fails. > + return NULL; > + } > + arglist = PyTuple_Pack(1, arg); > + Py_DECREF(arg); > + if (!arglist) { > + return NULL; > + } > + result = PyObject_CallObject(matchfn, arglist); > Py_DECREF(arglist); > - Py_DECREF(arg); > /* if the callback raised an exception, just let it > * through and give up */ > if (!result) { > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > https://selenic.com/mailman/listinfo/mercurial-devel >
On Wed, 06 Jan 2016 05:58:26 +0000, Martin von Zweigbergk wrote: > On Tue, Jan 5, 2016 at 8:01 AM Augie Fackler <raf@durin42.com> wrote: > > > # HG changeset patch > > # User Augie Fackler <raf@durin42.com> > > # Date 1451586702 18000 > > # Thu Dec 31 13:31:42 2015 -0500 > > # Node ID 700ae5816d662e0e5dde55c68944329b6eb1b7c0 > > # Parent 0bc71f45d3623b231cc3975b48feccce79d1231e > > lazymanifest: check more return values in filtercopy > > > > Spotted by Bryan O'Sullivan (and vexingly not the static analyzer I've > > been using.) > > > > diff --git a/mercurial/manifest.c b/mercurial/manifest.c > > --- a/mercurial/manifest.c > > +++ b/mercurial/manifest.c > > @@ -707,11 +707,20 @@ static lazymanifest *lazymanifest_filter > > copy->pydata = self->pydata; > > Py_INCREF(self->pydata); > > for (i = 0; i < self->numlines; i++) { > > - PyObject *arg = PyString_FromString(self->lines[i].start); > > - PyObject *arglist = PyTuple_Pack(1, arg); > > - PyObject *result = PyObject_CallObject(matchfn, arglist); > > + PyObject *arg = NULL, *arglist = NULL, *result = NULL; > > + arg = PyString_FromString(self->lines[i].start); > > + if (!arg) { > > + PyErr_SetString(PyExc_TypeError, > > + "couldn't pack filename"); > > > > TypeError seems wrong here. Most likely out of memory? Should we just leave > it as whatever it was already set to? We don't set it when PyTuple_Pack() > below fails. > > > > + return NULL; > > + } > > + arglist = PyTuple_Pack(1, arg); > > + Py_DECREF(arg); > > + if (!arglist) { > > + return NULL; > > + } Perhaps we can use Py_BuildValue("(s)", s) to avoid ugly error handling. https://docs.python.org/2/c-api/arg.html#c.Py_BuildValue
> On Jan 6, 2016, at 12:58 AM, Martin von Zweigbergk <martinvonz@google.com> wrote: > > + arg = PyString_FromString(self->lines[i].start); > + if (!arg) { > + PyErr_SetString(PyExc_TypeError, > + "couldn't pack filename"); > > TypeError seems wrong here. Most likely out of memory? Should we just leave it as whatever it was already set to? We don't set it when PyTuple_Pack() below fails. I’m fine with some other error type. My reading of the docs didn’t suggest PyString_FromString *would* set some sort of error condition, and I’m being paranoid at this point. (The docs state that it’ll set a TypeError if it get’s a non-string param, but it’s unclear to me what other failure states will produce. I suppose I could read the source…)
> On Jan 6, 2016, at 8:56 AM, Yuya Nishihara <yuya@tcha.org> wrote: > > > Perhaps we can use Py_BuildValue("(s)", s) to avoid ugly error handling. > > https://docs.python.org/2/c-api/arg.html#c.Py_BuildValue Nice! I’ll send a v2 with that.
Patch
diff --git a/mercurial/manifest.c b/mercurial/manifest.c --- a/mercurial/manifest.c +++ b/mercurial/manifest.c @@ -707,11 +707,20 @@ static lazymanifest *lazymanifest_filter copy->pydata = self->pydata; Py_INCREF(self->pydata); for (i = 0; i < self->numlines; i++) { - PyObject *arg = PyString_FromString(self->lines[i].start); - PyObject *arglist = PyTuple_Pack(1, arg); - PyObject *result = PyObject_CallObject(matchfn, arglist); + PyObject *arg = NULL, *arglist = NULL, *result = NULL; + arg = PyString_FromString(self->lines[i].start); + if (!arg) { + PyErr_SetString(PyExc_TypeError, + "couldn't pack filename"); + return NULL; + } + arglist = PyTuple_Pack(1, arg); + Py_DECREF(arg); + if (!arglist) { + return NULL; + } + result = PyObject_CallObject(matchfn, arglist); Py_DECREF(arglist); - Py_DECREF(arg); /* if the callback raised an exception, just let it * through and give up */ if (!result) {