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

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2017, 22:51 UTC
Message-ID: <xmqq4lwt1w54.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.21.1.1705091540130.146734@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 18 quoted lines

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

Unless I from this year is failing to spot a breakage that will be caused in the codepath that I from year 2008 and René spotted, I think freeing ce after we are done updating the working tree file with it is safe. I'd need to find time to make sure, though.

Show 7 quoted lines

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

I think it would be better to leave the fix as a separate patch. It wasn't spotted by Coverity in the first place ;-)