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

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 02.03.2012 20:17, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:
> 
>> which shows some parts of unpack-trees.c that I use as context to
>> ask: Should we check for o->merge in line 775, before using src[0]?
>>
>> If o->merge is 0, the src[0] will be NULL right up to the call of
>> unpack_nondirectories() in line 772.  There it may be set (in line
>> 582).  In that case we'll end up at line 779, where mark_ce_used()
>> is applied to it.
>>
>> I suspect that this is unintended and that line 775 should rather
>> read "if (o->merge&&  src[0]) {".  Can someone with a better
>> understanding of unpack-trees confirm or refute that suspicion?
> 
> Yeah, src[0] is meant to hold the entry from the current index to take it
> as well as our tree into account during o->merge, and I do not think it
> should affect when we are only reading tree(s) into the index.
> 
> I think da165f4 (unpack-trees.c: prepare for looking ahead in the index,
> 2010-01-07) simply forgot that the codepath also has to work when it is
> not merging.
> 
> Having said that, I do not know offhand if we just should nothing in
> no-merge case, or we should be doing something else instead, without
> thinking a bit more.

Thanks.

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().

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.  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.

To clean up after ourselves, explicitly loop through the entries and
free their memory for merges.  For non-merges, split out the actual
addition from add_entry() into the new helper do_add_entry().  Then
call that non-duplicating function instead of add_entry() to avoid the
leak.

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

diff --git a/unpack-trees.c b/unpack-trees.c
index 7c9ecf6..c594e4a 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);
 }
 
 /*
@@ -582,12 +589,18 @@ static int unpack_nondirectories(int n, unsigned long mask,
 		src[i + o->merge] = create_ce_entry(info, names + i, stage);
 	}
 
-	if (o->merge)
-		return call_unpack_fn(src, o);
+	if (o->merge) {
+		int rc = call_unpack_fn(src, o);
+		for (i = 0; i < n; i++) {
+			if (src[i + 1] != o->df_conflict_entry)
+				free(src[i + 1]);
+		}
+		return rc;
+	}
 
 	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