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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]

Hi Junio & René,

On Mon, 8 May 2017, Junio C Hamano wrote:

> René Scharfe <l.s.r@web.de> writes:
> 
> >>   	/*
> >>   	 * 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.
> 
> Yup, you should have told that to me back in Aug 2008 ;-) when I did
> 0cf8581e ("checkout -m: recreate merge when checking out of unmerged
> index", 2008-08-30); pretend_sha1_file() was available since early
> 2007, and there is no excuse that this codepath did not use it.

I hope y'all agree that this is outside the scope of my patch series...

> >> @@ -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).
> 
> Yes.
> 
> In the very old days it was not allowed to free(3) contents of
> active_cache[] and this was an old brain fart that came out of
> inertia.  We are manufacturing a brand new ce, only to feed it to
> checkout_entry() without even registering it to the active_cache[]
> array, and the ancient rule doesn't even apply to such a case.
> 
> So I think it was safe to free(3) even back then.

So this patch is good, then?

> > And result_buf is still leaked here, right?
> 
> Good spotting.  It typically would make a larger leak than a single
> ce, I would suppose ;-)

I saw you added this as a fixup! commit. If you don't mind, and if no
other changes are requested, would you mind rebase'ing this yourself?

Thanks,
Dscho