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

From: Jeff King <peff@peff.net>
Date: Mar 8, 2021, 19:12 UTC
Message-ID: <YEZ3Gf0f/NfXiwW2@coredump.intra.peff.net>
In-Reply-To: <107e98d00e1685e0144ee2415b8220c53542e5a3.1615228580.git.gitgitgadget@gmail.com>

On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:

Show 8 quoted lines

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

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

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

> +	UNLEAK(repo);

This one could be done with the idiom I mentioned earlier:

  repo = absolute_repo = absolute_pathdup(repo_name);
  ...
  free(absolute_repo);

but I think this is a perfect use of UNLEAK().

-Peff