Patchwork [manifest-leak-fix] lazymanifest: prevent leak when updating an entry more than once

login
register
mail settings
Submitter Augie Fackler
Date April 11, 2015, 3:58 p.m.
Message ID <1ae719dda0da9f935e7f.1428767893@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/8617/
State Accepted
Commit 84795da945d6511d5c34b02ae8bb4b4c5b10eb6a
Headers show

Comments

Augie Fackler - April 11, 2015, 3:58 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1428767781 14400
#      Sat Apr 11 11:56:21 2015 -0400
# Node ID 1ae719dda0da9f935e7fb49681c3b6467677df1d
# Parent  fbdbff1b486a5b9c12c0cebf4d172be90a2bb5f8
lazymanifest: prevent leak when updating an entry more than once

__setitem__ on the lazymanifest C type wasn't checking to see if a
line had previously been malloced before replacing it, leading to
leaks if files got updated multiple times in the course of a task.

I was able to reproduce the leak with this change to test-manifest.py:
Martin von Zweigbergk - April 11, 2015, 4:09 p.m.
On Sat, Apr 11, 2015 at 8:59 AM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1428767781 14400
> #      Sat Apr 11 11:56:21 2015 -0400
> # Node ID 1ae719dda0da9f935e7fb49681c3b6467677df1d
> # Parent  fbdbff1b486a5b9c12c0cebf4d172be90a2bb5f8
> lazymanifest: prevent leak when updating an entry more than once
>
> __setitem__ on the lazymanifest C type wasn't checking to see if a
> line had previously been malloced before replacing it, leading to
> leaks if files got updated multiple times in the course of a task.
>

The patch makes sense to me. Thanks for investigating and fixing!


>
> I was able to reproduce the leak with this change to test-manifest.py:
>
> diff --git a/tests/test-manifest.py b/tests/test-manifest.py
> --- a/tests/test-manifest.py
> +++ b/tests/test-manifest.py
> @@ -456,6 +456,16 @@ class basemanifesttests(object):
>                  ['a/b/c/bar.txt', 'a/b/c/foo.txt', 'a/b/d/ten.txt'],
>                  m2.keys())
>
> +    def testManifestSetItem(self):
> +        m = self.parsemanifest('')
> +        for x in range(3):
> +            m['file%d' % x] = BIN_HASH_1
> +        for x in range(3):
> +            m['file%d' % x] = BIN_HASH_2
> +        import time
> +        time.sleep(4)
> +
> +
>
> along with the commands:
>
>  $ make local
>  $ PYTHONPATH=. SILENT_BE_NOISY=1 python tests/test-manifest.py
> testmanifestdict.testManifestSetItem &
>  $ sleep 4
>  $ leaks $(jobs -p | tee /dev/stderr | awk '{print $3}')
>  $ wait
>
> in an interactive shell on OS X. As far as I can tell, it had to be an
> interactive shell so that I could get the pid of the test run using
> the jobs builtin. Prior to this change, I was leaking several strings,
> and after this change leaks reports no leaks.
>
> I thought there was a bug filed for this in bugzilla, but I can't find
> it either in bugzilla or by searching my email.
>

I only reported my suspicion of the bug on #mercurial. I saw it while
rebasing thousands of commits and had no simple reproduction scenario, so I
didn't file a bug.


>
> diff --git a/mercurial/manifest.c b/mercurial/manifest.c
> --- a/mercurial/manifest.c
> +++ b/mercurial/manifest.c
> @@ -440,6 +440,8 @@ static int internalsetitem(lazymanifest
>                 else {
>                         if (self->lines[pos].deleted)
>                                 self->livelines++;
> +                       if (self->lines[pos].from_malloc)
> +                               free(self->lines[pos].start);
>                         start = pos;
>                         goto finish;
>                 }
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Augie Fackler - April 11, 2015, 6:03 p.m.
On Sat, Apr 11, 2015 at 04:09:18PM +0000, Martin von Zweigbergk wrote:
> On Sat, Apr 11, 2015 at 8:59 AM Augie Fackler <raf@durin42.com> wrote:
>
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1428767781 14400
> > #      Sat Apr 11 11:56:21 2015 -0400
> > # Node ID 1ae719dda0da9f935e7fb49681c3b6467677df1d
> > # Parent  fbdbff1b486a5b9c12c0cebf4d172be90a2bb5f8
> > lazymanifest: prevent leak when updating an entry more than once
> >
> > __setitem__ on the lazymanifest C type wasn't checking to see if a
> > line had previously been malloced before replacing it, leading to
> > leaks if files got updated multiple times in the course of a task.
> >
>
> The patch makes sense to me. Thanks for investigating and fixing!

Thanks, I'll go ahead and push this.

>
>
> >
> > I was able to reproduce the leak with this change to test-manifest.py:
> >
> > diff --git a/tests/test-manifest.py b/tests/test-manifest.py
> > --- a/tests/test-manifest.py
> > +++ b/tests/test-manifest.py
> > @@ -456,6 +456,16 @@ class basemanifesttests(object):
> >                  ['a/b/c/bar.txt', 'a/b/c/foo.txt', 'a/b/d/ten.txt'],
> >                  m2.keys())
> >
> > +    def testManifestSetItem(self):
> > +        m = self.parsemanifest('')
> > +        for x in range(3):
> > +            m['file%d' % x] = BIN_HASH_1
> > +        for x in range(3):
> > +            m['file%d' % x] = BIN_HASH_2
> > +        import time
> > +        time.sleep(4)
> > +
> > +
> >
> > along with the commands:
> >
> >  $ make local
> >  $ PYTHONPATH=. SILENT_BE_NOISY=1 python tests/test-manifest.py
> > testmanifestdict.testManifestSetItem &
> >  $ sleep 4
> >  $ leaks $(jobs -p | tee /dev/stderr | awk '{print $3}')
> >  $ wait
> >
> > in an interactive shell on OS X. As far as I can tell, it had to be an
> > interactive shell so that I could get the pid of the test run using
> > the jobs builtin. Prior to this change, I was leaking several strings,
> > and after this change leaks reports no leaks.
> >
> > I thought there was a bug filed for this in bugzilla, but I can't find
> > it either in bugzilla or by searching my email.
> >
>
> I only reported my suspicion of the bug on #mercurial. I saw it while
> rebasing thousands of commits and had no simple reproduction scenario, so I
> didn't file a bug.
>
>
> >
> > diff --git a/mercurial/manifest.c b/mercurial/manifest.c
> > --- a/mercurial/manifest.c
> > +++ b/mercurial/manifest.c
> > @@ -440,6 +440,8 @@ static int internalsetitem(lazymanifest
> >                 else {
> >                         if (self->lines[pos].deleted)
> >                                 self->livelines++;
> > +                       if (self->lines[pos].from_malloc)
> > +                               free(self->lines[pos].start);
> >                         start = pos;
> >                         goto finish;
> >                 }
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> >

Patch

diff --git a/tests/test-manifest.py b/tests/test-manifest.py
--- a/tests/test-manifest.py
+++ b/tests/test-manifest.py
@@ -456,6 +456,16 @@  class basemanifesttests(object):
                 ['a/b/c/bar.txt', 'a/b/c/foo.txt', 'a/b/d/ten.txt'],
                 m2.keys())

+    def testManifestSetItem(self):
+        m = self.parsemanifest('')
+        for x in range(3):
+            m['file%d' % x] = BIN_HASH_1
+        for x in range(3):
+            m['file%d' % x] = BIN_HASH_2
+        import time
+        time.sleep(4)
+
+

along with the commands:

 $ make local
 $ PYTHONPATH=. SILENT_BE_NOISY=1 python tests/test-manifest.py testmanifestdict.testManifestSetItem &
 $ sleep 4
 $ leaks $(jobs -p | tee /dev/stderr | awk '{print $3}')
 $ wait

in an interactive shell on OS X. As far as I can tell, it had to be an
interactive shell so that I could get the pid of the test run using
the jobs builtin. Prior to this change, I was leaking several strings,
and after this change leaks reports no leaks.

I thought there was a bug filed for this in bugzilla, but I can't find
it either in bugzilla or by searching my email.

diff --git a/mercurial/manifest.c b/mercurial/manifest.c
--- a/mercurial/manifest.c
+++ b/mercurial/manifest.c
@@ -440,6 +440,8 @@  static int internalsetitem(lazymanifest 
 		else {
 			if (self->lines[pos].deleted)
 				self->livelines++;
+			if (self->lines[pos].from_malloc)
+				free(self->lines[pos].start);
 			start = pos;
 			goto finish;
 		}