Patchwork lazymanifest: check more return values in filtercopy

login
register
mail settings
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

Augie Fackler - Jan. 5, 2016, 3:57 p.m.
# 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.)
Martin von Zweigbergk - Jan. 6, 2016, 5:58 a.m.
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
>
Yuya Nishihara - Jan. 6, 2016, 1:56 p.m.
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
Augie Fackler - Jan. 6, 2016, 9:25 p.m.
> 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…)
Augie Fackler - Jan. 6, 2016, 9:25 p.m.
> 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) {