Re: [PATCH 03/28] fetch-pack: fix leaking sought refs

> From: Patrick Steinhardt <ps@pks.im>
>
> When calling `fetch_pack()` the caller is expected to pass in a set of
> sought-after refs that they want to fetch. This array gets massaged to
> not contain duplicate entries, which is done by replacing duplicate refs
> with `NULL` pointers. This modifies the caller-provided array, and in
> case we do unset any pointers the caller now loses track of that ref and
> cannot free it anymore.
>
> Now the obvious fix would be to not only unset these pointers, but to
> also free their contents. But this doesn't work because callers continue
> to use those refs. Another potential solution would be to copy the array
> in `fetch_pack()` so that we dont modify the caller-provided one. But
> that doesn't work either because the NULL-ness of those entries is used
> by callers to skip over ref entries that we didn't even try to fetch in
> `report_unmatched_refs()`.
>
> Instead, we make it the responsibility of our callers to duplicate these
> arrays as needed. It ain't pretty, but it works to plug the memory leak.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fetch-pack.c   | 11 ++++++++++-
>  t/t5700-protocol-v1.sh |  1 +
>  transport.c            | 11 ++++++++++-
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 49222a36fa..c5319232a5 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc,
>  	struct ref *fetched_refs = NULL, *remote_refs = NULL;
>  	const char *dest = NULL;
>  	struct ref **sought = NULL;
> +	struct ref **sought_to_free = NULL;
>  	int nr_sought = 0, alloc_sought = 0;
>  	int fd[2];
>  	struct string_list pack_lockfiles = STRING_LIST_INIT_DUP;
> @@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc,
>  		BUG("unknown protocol version");
>  	}
>
> +	/*
> +	 * Create a shallow copy of `sought` so that we can free all of its entries.
> +	 * This is because `fetch_pack()` will modify the array to evict some
> +	 * entries, but won't free those.
> +	 */
> +	DUP_ARRAY(sought_to_free, sought, nr_sought);
> +
>  	fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,
>  			 &shallow, pack_lockfiles_ptr, version);
>
> @@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc,
>  		       oid_to_hex(&ref->old_oid), ref->name);
>
>  	for (size_t i = 0; i < nr_sought; i++)
> -		free_one_ref(sought[i]);
> +		free_one_ref(sought_to_free[i]);
> +	free(sought_to_free);

>  	free(sought);
>  	free_refs(fetched_refs);
>  	free_refs(remote_refs);
> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
> index a73b4d4ff6..985e04d06e 100755
> --- a/t/t5700-protocol-v1.sh
> +++ b/t/t5700-protocol-v1.sh
> @@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  # Test protocol v1 with 'git://' transport
> diff --git a/transport.c b/transport.c
> index 3c4714581f..1098bbd60e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>  	struct git_transport_data *data = transport->data;
>  	struct ref *refs = NULL;
>  	struct fetch_pack_args args;
> -	struct ref *refs_tmp = NULL;
> +	struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;
>
>  	memset(&args, 0, sizeof(args));
>  	args.uploadpack = data->options.uploadpack;
> @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,
>  		goto cleanup;
>  	}
>
> +	/*
> +	 * Create a shallow copy of `sought` so that we can free all of its entries.
> +	 * This is because `fetch_pack()` will modify the array to evict some
> +	 * entries, but won't free those.
> +	 */
> +	DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);
> +	to_fetch = to_fetch_dup;
> +
>  	refs = fetch_pack(&args, data->fd,
>  			  refs_tmp ? refs_tmp : transport->remote_refs,
>  			  to_fetch, nr_heads, &data->shallow,
> @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>  		ret = -1;
>  	data->conn = NULL;
>
> +	free(to_fetch_dup);