Re: [PATCH v4 11/25] checkout: fix memory leak

["René Scharfe" <l.s.r@web.de>]

Am 04.05.2017 um 15:56 schrieb Johannes Schindelin:
> This change addresses part of the NEEDSWORK comment above the code,
> therefore the comment needs to be adjusted, too.
> 
> Discovered via Coverity.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   builtin/checkout.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f335..5faea3a05fa 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state)
>   	/*
>   	 * NEEDSWORK:
>   	 * There is absolutely no reason to write this as a blob object
> -	 * and create a phony cache entry just to leak.  This hack is
> -	 * primarily to get to the write_entry() machinery that massages
> -	 * the contents to work-tree format and writes out which only
> -	 * allows it for a cache entry.  The code in write_entry() needs
> -	 * to be refactored to allow us to feed a <buffer, size, mode>
> -	 * instead of a cache entry.  Such a refactoring would help
> -	 * merge_recursive as well (it also writes the merge result to the
> -	 * object database even when it may contain conflicts).
> +	 * and create a phony cache entry.  This hack is primarily to get
> +	 * to the write_entry() machinery that massages the contents to
> +	 * work-tree format and writes out which only allows it for a
> +	 * cache entry.  The code in write_entry() needs to be refactored
> +	 * to allow us to feed a <buffer, size, mode> instead of a cache
> +	 * entry.  Such a refactoring would help merge_recursive as well
> +	 * (it also writes the merge result to the object database even
> +	 * when it may contain conflicts).
>   	 */
>   	if (write_sha1_file(result_buf.ptr, result_buf.size,
>   			    blob_type, oid.hash))

Random observation: Using pretend_sha1_file here would at least avoid
writing the blob.

> @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state)
>   	if (!ce)
>   		die(_("make_cache_entry failed for path '%s'"), path);
>   	status = checkout_entry(ce, state, NULL);
> +	free(ce);
>   	return status;
>   }

I wonder if that's safe.  Why document a leak when it could have been
plugged this easily instead?  A leak is better than a use after free, so
let's be extra careful here.  Would it leave the index inconsistent?  Or
perhaps freeing it has become safe in the meantime?

@Junio: Do you remember the reason for the leaks in 0cf8581e330
(checkout -m: recreate merge when checking out of unmerged index).

And result_buf is still leaked here, right?

FWIW, after fixing a different issue (patch just sent separately)
t*checkout*.sh complete successfully for me with --valgrind, with and
without freeing both ce and result_buf.ptr.  So it *seems* safe..

René