Re: [PATCH 3/7] clone: free or UNLEAK further pointers when finished

From: Andrzej Hunt <andrzej@ahunt.org>
Date: Mar 14, 2021, 16:56 UTC
Message-ID: <9856ec4c-b8dc-3c93-ee20-f818672375b7@ahunt.org>
In-Reply-To: <YEZ3Gf0f/NfXiwW2@coredump.intra.peff.net>

On 08/03/2021 20:12, Jeff King wrote:

Show 14 quoted lines

> On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	repo_name = argv[0];
>>   
>>   	path = get_repo_path(repo_name, &is_bundle);
>> -	if (path)
>> +	if (path) {
>> +		free(path);
>>   		repo = absolute_pathdup(repo_name);
> 
> You mentioned that "path" gets reused again later. Should we use
> FREE_AND_NULL() to make sure that nobody tries to look at it in the
> meantime?

That sounds sensible - I wasn't too sure at first because we are unconditionally setting path later... but I can't see an reason not to make this safer.

But that makes me wonder if the definition of path should set it to NULL too. Setting it to NULL after freeing it guarantees that we can't read the old value anymore. However later code has no idea if path will be NULL or merely initialised (at least until we overwrite it with the new path).

I've provisionally updated my patch to also set path = NULL at point of definition, but I don't know if that's idiomatic in this scenario.

Show 13 quoted lines

> 
>> @@ -1393,6 +1394,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	strbuf_release(&reflog_msg);
>>   	strbuf_release(&branch_top);
>>   	strbuf_release(&key);
>> +	free_refs(mapped_refs);
>> +	free_refs((void *)remote_head_points_at);
> 
> We should avoid casting away constness when possible (because it is
> often a sign that sometimes the variable _isn't_ pointing to owned
> memory). In this case, I think freeing is the right thing; our
> guess_remote_head() returns a copy of the struct (which is non-const).
> Should remote_head_points_at just be declared without const?

I think so - I'll change remote_head_points_at to non-const.

Show 10 quoted lines

>> +	free_refs((void *)refs);
> 
> This one is more questionable to me. It comes from
> transport_get_remote_refs(), which does return a const pointer. And it
> looks like that memory is owned by the transport struct. So presumably
> we need to tell the transport code to clean itself up (or mark it with
> UNLEAK). Or perhaps there's a bug in the transport code (e.g., should it
> be freeing transport->remote_refs in transport_disconnect()? You'd want
> to make sure that no other callers expect the ref list to live on past
> the disconnect).

There are indeed multiple locations where we store the fetched refs, followed by transport_disconnect(), followed by trying to use the refs (that are nomimanlly owned by the now disconnected transport):

https://git.kernel.org/pub/scm/git/git.git/tree/builtin/remote.c?h=next#n953 https://git.kernel.org/pub/scm/git/git.git/tree/builtin/ls-remote.c?h=next#n122

However all other locations could handle free()'ing during transport_disconnect - and the 2 I've linked to above are easy enough to fix. So I'll give up on free_refs() from cmd_clone(), and will move it into transport_disconnect() as suggested. I've added a new patch to the end of the series to take care of this change.

(Which ultimately means we've now solved the same pattern of leak for all 5 users of transport_get_remote_refs().)