Re: [PATCH] ls-remote & transport API: release "struct transport_ls_refs_options"

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a memory leak in codepaths that use the "struct
> transport_ls_refs_options" API. Since the introduction of the struct
> in 39835409d10 (connect, transport: encapsulate arg in struct,
> 2021-02-05) the caller has been responsible for freeing it.
>
> That commit in turn migrated code originally added in
> 402c47d9391 (clone: send ref-prefixes when using protocol v2,
> 2018-07-20) and b4be74105fe (ls-remote: pass ref prefixes when
> requesting a remote's refs, 2018-03-15). Only some of those codepaths
> were releasing the allocated resources of the struct, now all of them
> will.
>
> Mark the "t/t5511-refspec.sh" test as passing when git is compiled
> with SANITIZE=leak. They'll now be listed as running under the
> "GIT_TEST_PASSING_SANITIZE_LEAK=true" test mode (the "linux-leaks" CI
> target). Previously 24/47 tests would fail.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/clone.c     | 13 ++++++-------
>  builtin/fetch.c     |  2 +-
>  builtin/ls-remote.c |  3 ++-
>  connect.c           |  4 ++--
>  t/t5511-refspec.sh  |  1 +
>  transport.c         |  8 +++++++-
>  transport.h         | 10 +++++++---
>  7 files changed, 26 insertions(+), 15 deletions(-)

This ...

> +void transport_ls_refs_options_release(struct transport_ls_refs_options *opts)
> +{
> +	strvec_clear(&opts->ref_prefixes);
> +	free((char *)opts->unborn_head_target);
> +}
> +

... addition is very much welcomed.  And instead of different code
paths doing "we used this member, so clear only that" ad-hoc, making
them all call it makes it very much pleasant read.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 727e16e0aea..8564e5f603f 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1233,7 +1233,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	}
>  	else {
>  		const char *branch;
> -		char *ref;
> +		const char *ref;
> +		char *ref_free = NULL;
>  
>  		if (option_branch)
>  			die(_("Remote branch %s not found in upstream %s"),
> @@ -1250,17 +1251,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		    skip_prefix(transport_ls_refs_options.unborn_head_target,
>  				"refs/heads/", &branch)) {
>  			ref = transport_ls_refs_options.unborn_head_target;
> -			transport_ls_refs_options.unborn_head_target = NULL;
>  			create_symref("HEAD", ref, reflog_msg.buf);
>  		} else {
>  			branch = git_default_branch_name(0);
> -			ref = xstrfmt("refs/heads/%s", branch);
> +			ref_free = xstrfmt("refs/heads/%s", branch);
> +			ref = ref_free;
>  		}
>  
>  		if (!option_bare)
>  			install_branch_config(0, branch, remote_name, ref);
> -
> -		free(ref);
> +		free(ref_free);
>  	}

It is a bit unfortunate that "ref" has to be sometimes a borrowed
pointer and some other times own the storage, only to allow us write
the call that uses the variable only once.  But under the
constraints of the current code, I think this is the best we could
do.

In our code base, we would usually call the auxiliary variable
"to_free", because its "ref"-ness does not matter and its sole
reason to exist is to be the "other owner" of the piece of memory,
to relieve the "ref" variable from the responsibility of releasing
resources.  With it, "ref" consistently borrows from somebody else,
either "to_free", or the .unborn_head_target member, and does not
have to be (and should not be) freed itself.