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

[Martin von Zweigbergk]

On Sat, Apr 11, 2015 at 8:59 AM Augie Fackler <raf at durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie at 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 at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20150411/625b3833/attachment-0002.html>