Re: [patch] possible memory leak in diff.c::diff_free_filepair()

[Junio C Hamano <junkio@cox.net>]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> oops.  probably my english wasn't clear. my patch fixes
> diff_free_filepair().

No, my reading of your patch when I wrote that message was
wrong.  The attached is what I ended up doing based on your
patch.  It does not seem to barf with the following test on
either git repository itself nor recent linux-2.6 repository,
which is a good sign.

    $ export MALLOC_CHECK_=2
    $ ./git-rev-list HEAD |
      ./git-diff-tree --stdin -r -B -C --find-copies-harder |
      sed -ne '/^[^:]/p;/ [MRCDA][0-9][0-9]*	/p'

When the command is run on linux-2.6 repository, virtual memory
consumption of git-diff-tree command skyrockets to about half a
gig, because it maps all files in two adjacent revisions of the
entire kernel tree.  But it seems to reclaim things reasonably
well and goes back down to less than 10m when it starts to
process the next commit pair.

------------
From: Yasushi SHOJI <yashi@atmark-techno.com>
Date: 1123930736 +0900
[PATCH] plug memory leak in diff.c::diff_free_filepair()

When I run git-diff-tree on big change, it seems the command eats so
much memory.  so I just put git under valgrind to see what's going on.
diff_free_filespec_data() doesn't free diff_filespec itself.

[jc: I ended up doing things slightly differently from Yasushi's
patch.  The original idea was to use free_filespec_data() only to
free the data portion and keep useing the filespec itself, but
no existing code seems to do things that way, so I just yanked
that part out.]

Signed-off-by: Yasushi SHOJI <yashi@atmark-techno.com>
Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 diff.c           |    9 ++++-----
 diffcore-break.c |    4 ++--
 diffcore.h       |    2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

20226fa40d48069b55cf165c9e197a003e1608a8
diff --git a/diff.c b/diff.c
--- a/diff.c
+++ b/diff.c
@@ -405,14 +405,13 @@ int diff_populate_filespec(struct diff_f
 	return 0;
 }
 
-void diff_free_filespec_data(struct diff_filespec *s)
+void diff_free_filespec(struct diff_filespec *s)
 {
 	if (s->should_free)
 		free(s->data);
 	else if (s->should_munmap)
 		munmap(s->data, s->size);
-	s->should_free = s->should_munmap = 0;
-	s->data = NULL;
+	free(s);
 }
 
 static void prep_temp_blob(struct diff_tempfile *temp,
@@ -769,8 +768,8 @@ struct diff_filepair *diff_queue(struct 
 
 void diff_free_filepair(struct diff_filepair *p)
 {
-	diff_free_filespec_data(p->one);
-	diff_free_filespec_data(p->two);
+	diff_free_filespec(p->one);
+	diff_free_filespec(p->two);
 	free(p);
 }
 
diff --git a/diffcore-break.c b/diffcore-break.c
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -231,8 +231,8 @@ static void merge_broken(struct diff_fil
 
 	dp = diff_queue(outq, d->one, c->two);
 	dp->score = p->score;
-	diff_free_filespec_data(d->two);
-	diff_free_filespec_data(c->one);
+	diff_free_filespec(d->two);
+	diff_free_filespec(c->one);
 	free(d);
 	free(c);
 }
diff --git a/diffcore.h b/diffcore.h
--- a/diffcore.h
+++ b/diffcore.h
@@ -43,7 +43,7 @@ extern void fill_filespec(struct diff_fi
 			  unsigned short);
 
 extern int diff_populate_filespec(struct diff_filespec *, int);
-extern void diff_free_filespec_data(struct diff_filespec *);
+extern void diff_free_filespec(struct diff_filespec *);
 
 struct diff_filepair {
 	struct diff_filespec *one;