Re: [RFH] unpack-trees: cache_entry lifetime issue?

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Mar 6, 2012, 19:37 UTC
Message-ID: <4F566773.5060005@lsrfire.ath.cx>
In-Reply-To: <7vaa3uzozy.fsf@alter.siamese.dyndns.org>

Am 05.03.2012 23:01, schrieb Junio C Hamano:

Show 10 quoted lines

> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
> 
>> Next question: Should the function fn() in struct unpack_trees_options
>> be able to replace src[0], and unpack_callback() is then supposed to
>> use the new pointer after calling unpack_nondirectories()?  If not
>> then we can clean up things a bit by moving the src array into
>> unpack_nondirectories().
> 
> Sorry, I have no idea.  What kind of usage pattern do you have in
> mind?

Well, I can't imagine a merge callback that needs this ability, but that really doesn't mean much. Just want to avoid taking away options that turn out to be useful later.

Show 14 quoted lines

>> For now, just this patch, which cleans up memory, but not the code:
>>
>> -- >8 --
>> Subject: unpack-trees: plug minor memory leak
>>
>> The allocations made by unpack_nondirectories() using create_ce_entry()
>> are never freed.  In the case of a merge, we hand them to
>> call_unpack_fn() and never look at them again.
> 
> That assumes that whatever callbacks that are called will only look
> at but never takes ownership of the cache entry given to them.  I
> *think* everybody eventually calls "add_entry()" that duplicates the
> cache entry before storing it to the index, but I didn't go through
> all the codepaths.  Assuming you did, I think this is a good change.

I did that a while back and while doing that again just now with tired eyes after a day's work noticed how deep the call chains are and that it's certainly possible that I missed a place that assumed it owned the cache entry.

For increased confidence, perhaps it's better to first patch the non-merge leak, then clean up and const'ify the merge case and only then free(). Or to declare that merge functions can take ownership of the cache entries. Timid patch for the first part below.

-- >8 --
Subject: unpack-trees: plug minor memory leak

The allocations made by unpack_nondirectories() using create_ce_entry() are never freed.

In the non-merge case, we duplicate them using add_entry() and later only look at the first allocated element (src[0]), perhaps even only by mistake. Split out the actual addition from add_entry() into the new helper do_add_entry() and call this non-duplicating function instead of add_entry() to avoid the leak.

Valgrind reports this for the command "git archive v1.7.9" without the patch:

  ==13372== LEAK SUMMARY:
  ==13372==    definitely lost: 230,986 bytes in 2,325 blocks
  ==13372==    indirectly lost: 0 bytes in 0 blocks
  ==13372==      possibly lost: 98 bytes in 1 blocks
  ==13372==    still reachable: 2,259,198 bytes in 3,243 blocks
  ==13372==         suppressed: 0 bytes in 0 blocks

And with the patch applied:

  ==13375== LEAK SUMMARY:
  ==13375==    definitely lost: 65 bytes in 1 blocks
  ==13375==    indirectly lost: 0 bytes in 0 blocks
  ==13375==      possibly lost: 0 bytes in 0 blocks
  ==13375==    still reachable: 2,364,417 bytes in 3,245 blocks
  ==13375==         suppressed: 0 bytes in 0 blocks

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 unpack-trees.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 7c9ecf6..f9c74bd 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -102,21 +102,28 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		opts->unpack_rejects[i].strdup_strings = 1;
 }
 
-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
-	unsigned int set, unsigned int clear)
+static void do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
+			 unsigned int set, unsigned int clear)
 {
-	unsigned int size = ce_size(ce);
-	struct cache_entry *new = xmalloc(size);
-
 	clear |= CE_HASHED | CE_UNHASHED;
 
 	if (set & CE_REMOVE)
 		set |= CE_WT_REMOVE;
 
+	ce->next = NULL;
+	ce->ce_flags = (ce->ce_flags & ~clear) | set;
+	add_index_entry(&o->result, ce,
+			ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
+}
+
+static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
+	unsigned int set, unsigned int clear)
+{
+	unsigned int size = ce_size(ce);
+	struct cache_entry *new = xmalloc(size);
+
 	memcpy(new, ce, size);
-	new->next = NULL;
-	new->ce_flags = (new->ce_flags & ~clear) | set;
-	add_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
+	do_add_entry(o, new, set, clear);
 }
 
 /*
@@ -587,7 +594,7 @@ static int unpack_nondirectories(int n, unsigned long mask,
 
 	for (i = 0; i < n; i++)
 		if (src[i] && src[i] != o->df_conflict_entry)
-			add_entry(o, src[i], 0, 0);
+			do_add_entry(o, src[i], 0, 0);
 	return 0;
 }

-- 
1.7.9.2