Patchwork [5,of,5,stable,cpychecker] osutil: fix memory leak of PyBytes of path in statfiles

login
register
mail settings
Submitter Augie Fackler
Date Jan. 27, 2015, 3:26 p.m.
Message ID <88c751a71e730c029a12.1422372396@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/7564/
State Superseded
Commit 2d2c0a8eeeb8764daa94400e7de16a02971361a9
Headers show

Comments

Augie Fackler - Jan. 27, 2015, 3:26 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1422371836 18000
#      Tue Jan 27 10:17:16 2015 -0500
# Branch stable
# Node ID 88c751a71e730c029a12b3664befa94ed62a9b7c
# Parent  6c71c9d9336df3a27325b956aeb70fe6d2d953d1
osutil: fix memory leak of PyBytes of path in statfiles

Spotted with cpychecker.
Martin von Zweigbergk - Jan. 27, 2015, 5:20 p.m.
This series looks good to me. I just have one question on the patch (see
below).

On Tue Jan 27 2015 at 7:30:58 AM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1422371836 18000
> #      Tue Jan 27 10:17:16 2015 -0500
> # Branch stable
> # Node ID 88c751a71e730c029a12b3664befa94ed62a9b7c
> # Parent  6c71c9d9336df3a27325b956aeb70fe6d2d953d1
> osutil: fix memory leak of PyBytes of path in statfiles
>
> Spotted with cpychecker.
>
> diff --git a/mercurial/osutil.c b/mercurial/osutil.c
> --- a/mercurial/osutil.c
> +++ b/mercurial/osutil.c
> @@ -410,17 +410,20 @@ static PyObject *statfiles(PyObject *sel
>                 return NULL;
>
>         for (i = 0; i < count; i++) {
> -               PyObject *stat;
> +               PyObject *stat, *pypath;
>                 struct stat st;
>                 int ret, kind;
>                 char *path;
>
> -               path = PyString_AsString(PySequence_GetItem(names, i));
> +               pypath = PySequence_GetItem(names, i);
> +               path = PyString_AsString(pypath);
>                 if (path == NULL) {
> +                       Py_DECREF(pypath);
>

What if pypath is NULL? Don't we need a separate check before creating
'path'?

(Technically, it might be okay to attempt to create 'path' from NULL, but
that might obscure the real error. We would also have to use XDECREF in
that case.)


>                         PyErr_SetString(PyExc_TypeError, "not a string");
>                         goto bail;
>                 }
>                 ret = lstat(path, &st);
> +               Py_DECREF(pypath);
>                 kind = st.st_mode & S_IFMT;
>                 if (ret != -1 && (kind == S_IFREG || kind == S_IFLNK)) {
>                         stat = makestat(&st);
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Augie Fackler - Jan. 28, 2015, 5:50 p.m.
On Tue, Jan 27, 2015 at 05:20:51PM +0000, Martin von Zweigbergk wrote:
> This series looks good to me. I just have one question on the patch (see
> below).
>
> On Tue Jan 27 2015 at 7:30:58 AM Augie Fackler <raf@durin42.com> wrote:
>
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1422371836 18000
> > #      Tue Jan 27 10:17:16 2015 -0500
> > # Branch stable
> > # Node ID 88c751a71e730c029a12b3664befa94ed62a9b7c
> > # Parent  6c71c9d9336df3a27325b956aeb70fe6d2d953d1
> > osutil: fix memory leak of PyBytes of path in statfiles
> >
> > Spotted with cpychecker.
> >
> > diff --git a/mercurial/osutil.c b/mercurial/osutil.c
> > --- a/mercurial/osutil.c
> > +++ b/mercurial/osutil.c
> > @@ -410,17 +410,20 @@ static PyObject *statfiles(PyObject *sel
> >                 return NULL;
> >
> >         for (i = 0; i < count; i++) {
> > -               PyObject *stat;
> > +               PyObject *stat, *pypath;
> >                 struct stat st;
> >                 int ret, kind;
> >                 char *path;
> >
> > -               path = PyString_AsString(PySequence_GetItem(names, i));
> > +               pypath = PySequence_GetItem(names, i);
> > +               path = PyString_AsString(pypath);
> >                 if (path == NULL) {
> > +                       Py_DECREF(pypath);
> >
>
> What if pypath is NULL? Don't we need a separate check before creating
> 'path'?

Good catch. Mailed a v2.

>
> (Technically, it might be okay to attempt to create 'path' from NULL, but
> that might obscure the real error. We would also have to use XDECREF in
> that case.)
>
>
> >                         PyErr_SetString(PyExc_TypeError, "not a string");
> >                         goto bail;
> >                 }
> >                 ret = lstat(path, &st);
> > +               Py_DECREF(pypath);
> >                 kind = st.st_mode & S_IFMT;
> >                 if (ret != -1 && (kind == S_IFREG || kind == S_IFLNK)) {
> >                         stat = makestat(&st);
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> >

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/osutil.c b/mercurial/osutil.c
--- a/mercurial/osutil.c
+++ b/mercurial/osutil.c
@@ -410,17 +410,20 @@  static PyObject *statfiles(PyObject *sel
 		return NULL;
 
 	for (i = 0; i < count; i++) {
-		PyObject *stat;
+		PyObject *stat, *pypath;
 		struct stat st;
 		int ret, kind;
 		char *path;
 
-		path = PyString_AsString(PySequence_GetItem(names, i));
+		pypath = PySequence_GetItem(names, i);
+		path = PyString_AsString(pypath);
 		if (path == NULL) {
+			Py_DECREF(pypath);
 			PyErr_SetString(PyExc_TypeError, "not a string");
 			goto bail;
 		}
 		ret = lstat(path, &st);
+		Py_DECREF(pypath);
 		kind = st.st_mode & S_IFMT;
 		if (ret != -1 && (kind == S_IFREG || kind == S_IFLNK)) {
 			stat = makestat(&st);