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

[Andrzej Hunt <andrzej@ahunt.org>]

On 08/03/2021 20:12, Jeff King wrote:
> 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.

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

>> +	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().)