Am 24.09.24 um 23:51 schrieb Jeff King:
> From: Patrick Steinhardt <ps@xxxxxx>
>
> 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@xxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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);
OK.
> 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);
This makes a shallow copy and passes it to fetch_pack() and later to
report_unmatched_refs(). It shields callers of fetch_refs_via_pack()
from the effect of fetch_pack()'s NULLification. This is what the
commit message says would not work. What am I missing?
> free_refs(refs_tmp);
> free_refs(refs);
> list_objects_filter_release(&args.filter_options);
