Am 02.03.2012 20:17, schrieb Junio C Hamano:
> René Scharfe<rene.scharfe@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html