Patchwork manifest.c needs an eyeball or two

login
register
mail settings
Submitter Augie Fackler
Date Dec. 31, 2015, 7 p.m.
Message ID <CFA8BDDF-4FC4-477B-B3A5-164F364B9A51@durin42.com>
Download mbox | patch
Permalink /patch/12448/
State Not Applicable
Headers show

Comments

Augie Fackler - Dec. 31, 2015, 7 p.m.
I clearly have a case of the dumb. I tried to fix this, and all I can do is get segfaults out the other end. Here’s what I’ve got after some beating my head against the wall:


(I found a handful of other non-checked error returns, but this one is causing problems if I try and fix it.)

> On Dec 12, 2015, at 11:07 PM, Bryan O'Sullivan <bos@serpentine.com> wrote:
> 
> Looking at use of some Python/C API functions, there are a few return values unchecked.
> 
> For instance, the first call to PyString_FromString in lazymanifest_filtercopy.
Martin von Zweigbergk - Jan. 1, 2016, 3:59 a.m.
I don't know what error you are seeing, but it looks like you have two
Py_DECREF(arg) there now. Could that be the problem?

On Thu, Dec 31, 2015, 11:01 Augie Fackler <raf@durin42.com> wrote:

> I clearly have a case of the dumb. I tried to fix this, and all I can do
> is get segfaults out the other end. Here’s what I’ve got after some beating
> my head against the wall:
>
> diff --git a/mercurial/manifest.c b/mercurial/manifest.c
> --- a/mercurial/manifest.c
> +++ b/mercurial/manifest.c
> @@ -704,9 +704,19 @@ 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
>
> (I found a handful of other non-checked error returns, but this one is
> causing problems if I try and fix it.)
>
> > On Dec 12, 2015, at 11:07 PM, Bryan O'Sullivan <bos@serpentine.com>
> wrote:
> >
> > Looking at use of some Python/C API functions, there are a few return
> values unchecked.
> >
> > For instance, the first call to PyString_FromString in
> lazymanifest_filtercopy.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Bryan O'Sullivan - Jan. 1, 2016, 9:28 p.m.
On Thu, Dec 31, 2015 at 7:59 PM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

> I don't know what error you are seeing, but it looks like you have two
> Py_DECREF(arg) there now. Could that be the problem?
>

That seems likely to me.
Augie Fackler - Jan. 5, 2016, 3:53 p.m.
> On Jan 1, 2016, at 4:28 PM, Bryan O'Sullivan <bos@serpentine.com> wrote:
> 
> 
> On Thu, Dec 31, 2015 at 7:59 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> I don't know what error you are seeing, but it looks like you have two Py_DECREF(arg) there now. Could that be the problem?
> 
> 
> That seems likely to me.

Ouch. Can’t believe I missed that. Thanks both!

Patch

diff --git a/mercurial/manifest.c b/mercurial/manifest.c
--- a/mercurial/manifest.c
+++ b/mercurial/manifest.c
@@ -704,9 +704,19 @@  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