Re: [RFH] unpack-trees: cache_entry lifetime issue?
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 5, 2012, 22:01 UTC
- Message-ID
- <7vaa3uzozy.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4F522758.9050205@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 5 quoted lines
> 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?
Show 8 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.
Show 77 quoted lines
> 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;
> }