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

[Junio C Hamano <gitster@pobox.com>]

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?

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

> 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;
>  }