@matheusbsbs / matheus-bagliano / commits / 6f54d00439

fetch-pack: fix leaking sought refs

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Sep 24, 2024 at 17:51 UTC 6f54d00439759feab82d40577d272c57d449a554

3 files changed +21 -2

builtin/fetch-pack.c

+10 -1

@@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc,

53 struct ref *fetched_refs = NULL, *remote_refs = NULL;

54 const char *dest = NULL;

55 struct ref **sought = NULL;

56 + struct ref **sought_to_free = NULL;

57 int nr_sought = 0, alloc_sought = 0;

58 int fd[2];

59 struct string_list pack_lockfiles = STRING_LIST_INIT_DUP;

@@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc,

244 BUG("unknown protocol version");

245 }

246

247 + /*

248 + * Create a shallow copy of `sought` so that we can free all of its entries.

249 + * This is because `fetch_pack()` will modify the array to evict some

250 + * entries, but won't free those.

251 + */

252 + DUP_ARRAY(sought_to_free, sought, nr_sought);

253 +

254 fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,

255 &shallow, pack_lockfiles_ptr, version);

256

@@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc,

288 oid_to_hex(&ref->old_oid), ref->name);

289

290 for (size_t i = 0; i < nr_sought; i++)

283 - free_one_ref(sought[i]);

291 + free_one_ref(sought_to_free[i]);

292 + free(sought_to_free);

293 free(sought);

294 free_refs(fetched_refs);

295 free_refs(remote_refs);

t/t5700-protocol-v1.sh

@@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION

11 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

12 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

14 +TEST_PASSES_SANITIZE_LEAK=true

15 . ./test-lib.sh

17 # Test protocol v1 with 'git://' transport

transport.c

+10 -1

@@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,

414 struct git_transport_data *data = transport->data;

415 struct ref *refs = NULL;

416 struct fetch_pack_args args;

417 - struct ref *refs_tmp = NULL;

417 + struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;

418

419 memset(&args, 0, sizeof(args));

420 args.uploadpack = data->options.uploadpack;

@@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,

477 goto cleanup;

478 }

479

480 + /*

481 + * Create a shallow copy of `sought` so that we can free all of its entries.

482 + * This is because `fetch_pack()` will modify the array to evict some

483 + * entries, but won't free those.

484 + */

485 + DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);

486 + to_fetch = to_fetch_dup;

487 +

488 refs = fetch_pack(&args, data->fd,

489 refs_tmp ? refs_tmp : transport->remote_refs,

490 to_fetch, nr_heads, &data->shallow,

@@ -500,6 +508,7 @@ cleanup:

508 ret = -1;

509 data->conn = NULL;

510

511 + free(to_fetch_dup);

512 free_refs(refs_tmp);

513 free_refs(refs);

514 list_objects_filter_release(&args.filter_options);