fetch-pack: fix segfault when fscking without --lock-pack

The fetch-pack internals have multiple options related to creating
".keep" lock-files for the received pack:

  - if args.lock_pack is set, then we tell index-pack to create a .keep
    file. In the fetch-pack plumbing command, this is triggered by
    passing "-k" twice.

  - if the caller passes in a pack_lockfiles string list, then we use it
    to record the path of the keep-file created by index-pack. We get
    that name by reading the stdout of index-pack. In the fetch-pack
    command, this is triggered by passing the (undocumented) --lock-pack
    option; without it, we pass in a NULL string list.

So it's possible to ask index-pack to create the lock-file (using "-k
-k") but not ask to record it (by avoiding "--lock-pack"). This worked
fine until 5476e1efde (fetch-pack: print and use dangling .gitmodules,
2021-02-22), but now it causes a segfault.

Before that commit, if pack_lockfiles was NULL, we wouldn't bother
reading the output from index-pack at all. But since that commit,
index-pack may produce extra output if we asked it to fsck. So even if
nobody cares about the lockfile path, we still need to read it to skip
to the output we do care about.

We correctly check that we didn't get a NULL lockfile path (which can
happen if we did not ask it to create a .keep file at all), but we
missed the case where the lockfile path is not NULL (due to "-k -k") but
the pack_lockfiles string_list is NULL (because nobody passed
"--lock-pack"), and segfault trying to add to the NULL string-list.

We can fix this by skipping the append to the string list when either
the value or the list is NULL. In that case we must also free the
lockfile path to avoid leaking it when it's non-NULL.

Nobody noticed the bug for so long because the transport code used by
"git fetch" always passes in a pack_lockfiles pointer, and remote-curl
(the main user of the fetch-pack plumbing command) always passes
--lock-pack.

Reported-by: Kirill Smelkov <kirr@nexedi.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fetch-pack.c b/fetch-pack.c
index 091f9a80a9ee72593c5079b48f15ea22dbd59d4c..733daa5da459e59b76111c04edd41e9b03b59ea0 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1041,8 +1041,10 @@ static int get_pack(struct fetch_pack_args *args,
 
                if (!is_well_formed)
                        die(_("fetch-pack: invalid index-pack output"));
-               if (pack_lockfile)
+               if (pack_lockfiles && pack_lockfile)
                        string_list_append_nodup(pack_lockfiles, pack_lockfile);
+               else
+                       free(pack_lockfile);
                parse_gitmodules_oids(cmd.out, gitmodules_oids);
                close(cmd.out);
        }

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 1bc15a3f080d3930f4f726ef7ad2bb9de38d368e..1b212d155e2363c56249bea0669a939c96f73a29 100755 (executable)
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -993,6 +993,16 @@ test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
                       fetch origin server_has both_have_2
 '
 
+test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault' '
+       rm -rf server client &&
+       test_create_repo server &&
+       test_commit -C server one &&
+
+       test_create_repo client &&
+       git -c fetch.fsckObjects=true \
+           -C client fetch-pack -k -k ../server HEAD
+'
+
 test_expect_success 'filtering by size' '
        rm -rf server client &&
        test_create_repo server &&