Re: [RFH] unpack-trees: cache_entry lifetime issue?
- From
René Scharfe <rene.scharfe@lsrfire.ath.cx>
- Date
- Mar 3, 2012, 14:14 UTC
- Message-ID
- <4F522758.9050205@lsrfire.ath.cx>
- In-Reply-To
- <7vk432dd89.fsf@alter.siamese.dyndns.org>
Am 02.03.2012 20:17, schrieb Junio C Hamano:
Show 25 quoted lines
> 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