| 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
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 >
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; }