{"thread":{"id":"61597","subject":"[PATCH] fetch-pack: test: demonstrate segmentation fault when run with fsckObjects but without --lock-pack","startedAt":"2024-06-06T13:36:36Z","lastAt":"2024-06-20T17:57:38Z","messageCount":7,"participants":["Kirill Smelkov","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496521","messageId":"20240606133605.602276-1-kirr@nexedi.com","threadId":"61597","inReplyTo":null,"subject":"[PATCH] fetch-pack: test: demonstrate segmentation fault when run with fsckObjects but without --lock-pack","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2024-06-06T13:36:35Z","receivedAt":"2024-06-06T13:36:36Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"When running git-backup[1] over lab.nexedi.com archive we noticed that\nGit started to crash on fetch-pack operation[2] after Git upgrade. We\ntracked this down to be a regression introduced by Git 2.31, more\nspecifically by commit 5476e1efded5 (fetch-pack: print and use dangling\n.gitmodules), which started to index and lock packfiles on do_keep=y &&\nfsck_objects=y even if pack_lockfiles=NULL, which leads to\nNULL-dereference when trying to append an entry to that pack_lockfiles\nstringlist.\n\nPlease find attached a test that demonstrates this problem.\n\nWhen run with test_expect_failure changed to test_expect_success the test\ncrashes with\n\n    ./test-lib.sh: line 1063: 599675 Segmentation fault      (core dumped) git -c fetch.fsckObjects=true -c fetch.unpackLimit=1 -C client fetch-pack ../server $(git -C server rev-parse refs/heads/main)\n\nand the backtrace is\n\n    Program terminated with signal SIGSEGV, Segmentation fault.\n    #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,\n        string=0x558033b695f0 \".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep\") at string-list.c:218\n    218             ALLOC_GROW(list->items, list->nr + 1, list->alloc);\n    (gdb) bt\n    #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,\n        string=0x558033b695f0 \".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep\") at string-list.c:218\n    #1  0x0000558032b5b83f in get_pack (args=0x7ffe680f3fa0, xd=0x7ffe680f4058, pack_lockfiles=0x0, index_pack_args=0x0,\n        sought=0x558033b65e90, nr_sought=1, gitmodules_oids=0x558032e17b88 <fsck_options+72>) at fetch-pack.c:1042\n    #2  0x0000558032b5e0b3 in do_fetch_pack_v2 (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, orig_ref=0x558033b67b90, sought=0x558033b65e90,\n        nr_sought=1, shallows=0x7ffe680f3df0, si=0x7ffe680f3e10, pack_lockfiles=0x0) at fetch-pack.c:1781\n    #3  0x0000558032b5ef3c in fetch_pack (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, ref=0x558033b67b90, sought=0x558033b65e90, nr_sought=1,\n        shallow=0x7ffe680f3f80, pack_lockfiles=0x0, version=protocol_v2) at fetch-pack.c:2073\n    #4  0x0000558032a08851 in cmd_fetch_pack (argc=3, argv=0x7ffe680f4500, prefix=0x0) at builtin/fetch-pack.c:242\n    #5  0x00005580329b2be3 in run_builtin (p=0x558032e0bb30 <commands+1008>, argc=3, argv=0x7ffe680f4500) at git.c:474\n    #6  0x00005580329b2ffe in handle_builtin (argc=3, argv=0x7ffe680f4500) at git.c:729\n    #7  0x00005580329b3222 in run_argv (argcp=0x7ffe680f433c, argv=0x7ffe680f4330) at git.c:793\n    #8  0x00005580329b3796 in cmd_main (argc=3, argv=0x7ffe680f4500) at git.c:928\n    #9  0x0000558032ab9002 in main (argc=10, argv=0x7ffe680f44c8) at common-main.c:62\n\nA simple debug patch below\n\n    --- a/fetch-pack.c\n    +++ b/fetch-pack.c\n    @@ -1032,7 +1032,7 @@ static int get_pack(struct fetch_pack_args *args,\n            cmd.git_cmd = 1;\n            if (start_command(&cmd))\n                    die(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n    -       if (do_keep && (pack_lockfiles || fsck_objects)) {\n    +       if (do_keep && (pack_lockfiles /*|| fsck_objects*/)) {\n                    int is_well_formed;\n                    char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);\n\nmakes the crash go away, but I did not investigated what should be the\nproper logic.\n\nThanks beforehand for fixing this NULL-pointer dereference.\n\nKirill\n\n[1] https://lab.nexedi.com/kirr/git-backup\n[2] https://lab.nexedi.com/kirr/git-backup/-/blob/3230197c/git-backup.go#L717-739\n\nCc: Jonathan Tan <jonathantanmy@google.com>\nCc: Alain Takoudjou <alain.takoudjou@nexedi.com>\nCc: Jérome Perrin <jerome@nexedi.com>\nSigned-off-by: Kirill Smelkov <kirr@nexedi.com>\n---\n t/t5500-fetch-pack.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 1bc15a3f08..e3dbe79613 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -1056,6 +1056,20 @@ test_expect_success 'fetch with --filter=blob:limit=0' '\n \tfetch_filter_blob_limit_zero server server\n '\n \n+test_expect_failure 'fetch with fsckObjects but without --lock-pack does not segfault' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\t# unpackLimit=1 forces to keep received pack as pack instead of\n+\t# unpacking it to loose objects. The code is currently segfaulting when\n+\t# trying to index that kept pack.\n+\tgit -c fetch.fsckObjects=true -c fetch.unpackLimit=1 \\\n+\t    -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/main)\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.39.2\n\n"},{"id":"497330","messageId":"ZnJpJyLPAKlu82s8@deca.navytux.spb.ru","threadId":"61597","inReplyTo":"20240606133605.602276-1-kirr@nexedi.com","subject":"[RESEND, BUG, SIGSEGV CRASH] (Re: [PATCH] fetch-pack: test: demonstrate segmentation fault when run with fsckObjects but without --lock-pack)","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2024-06-19T05:14:21Z","receivedAt":"2024-06-19T05:14:22Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"+ newren, peff, calvinwan, ps, avarab\n\nHello everyone. This patch demonstrates *segmentation fault* crash of Git\nwith, hopefully, properly written test. It was originally posted two\nweeks ago without getting any replies nor any traces in seen/todo:\n\nhttps://lore.kernel.org/git/20240606133605.602276-1-kirr@nexedi.com/T/#m9d454aadc8857b84f17d1a331739f7399cf1bbf8\n\nI understand that there might be something wrong with my test, but could\nyou please at least provide some feedback and acknowledge the problem?\n\nResending the patch and adding more people to Cc who touched fetch-pack.c recently.\n\nThanks beforehand for feedback,\nKirill\n\n---- 8< ----\nFrom: Kirill Smelkov <kirr@nexedi.com>\nDate: Thu, 6 Jun 2024 16:03:44 +0300\nSubject: [PATCH] fetch-pack: test: demonstrate segmentation fault when run\n with fsckObjects but without --lock-pack\nMIME-Version: 1.0\nContent-Type: text/plain; charset=utf-8\nContent-Transfer-Encoding: 8bit\n\nWhen running git-backup[1] over lab.nexedi.com archive we noticed that\nGit started to crash on fetch-pack operation[2] after Git upgrade. We\ntracked this down to be a regression introduced by Git 2.31, more\nspecifically by commit 5476e1efded5 (fetch-pack: print and use dangling\n.gitmodules), which started to index and lock packfiles on do_keep=y &&\nfsck_objects=y even if pack_lockfiles=NULL, which leads to\nNULL-dereference when trying to append an entry to that pack_lockfiles\nstringlist.\n\nPlease find attached a test that demonstrates this problem.\n\nWhen run with test_expect_failure changed to test_expect_success the test\ncrashes with\n\n    ./test-lib.sh: line 1063: 599675 Segmentation fault      (core dumped) git -c fetch.fsckObjects=true -c fetch.unpackLimit=1 -C client fetch-pack ../server $(git -C server rev-parse refs/heads/main)\n\nand the backtrace is\n\n    Program terminated with signal SIGSEGV, Segmentation fault.\n    #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,\n        string=0x558033b695f0 \".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep\") at string-list.c:218\n    218             ALLOC_GROW(list->items, list->nr + 1, list->alloc);\n    (gdb) bt\n    #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,\n        string=0x558033b695f0 \".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep\") at string-list.c:218\n    #1  0x0000558032b5b83f in get_pack (args=0x7ffe680f3fa0, xd=0x7ffe680f4058, pack_lockfiles=0x0, index_pack_args=0x0,\n        sought=0x558033b65e90, nr_sought=1, gitmodules_oids=0x558032e17b88 <fsck_options+72>) at fetch-pack.c:1042\n    #2  0x0000558032b5e0b3 in do_fetch_pack_v2 (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, orig_ref=0x558033b67b90, sought=0x558033b65e90,\n        nr_sought=1, shallows=0x7ffe680f3df0, si=0x7ffe680f3e10, pack_lockfiles=0x0) at fetch-pack.c:1781\n    #3  0x0000558032b5ef3c in fetch_pack (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, ref=0x558033b67b90, sought=0x558033b65e90, nr_sought=1,\n        shallow=0x7ffe680f3f80, pack_lockfiles=0x0, version=protocol_v2) at fetch-pack.c:2073\n    #4  0x0000558032a08851 in cmd_fetch_pack (argc=3, argv=0x7ffe680f4500, prefix=0x0) at builtin/fetch-pack.c:242\n    #5  0x00005580329b2be3 in run_builtin (p=0x558032e0bb30 <commands+1008>, argc=3, argv=0x7ffe680f4500) at git.c:474\n    #6  0x00005580329b2ffe in handle_builtin (argc=3, argv=0x7ffe680f4500) at git.c:729\n    #7  0x00005580329b3222 in run_argv (argcp=0x7ffe680f433c, argv=0x7ffe680f4330) at git.c:793\n    #8  0x00005580329b3796 in cmd_main (argc=3, argv=0x7ffe680f4500) at git.c:928\n    #9  0x0000558032ab9002 in main (argc=10, argv=0x7ffe680f44c8) at common-main.c:62\n\nA simple debug patch below\n\n    --- a/fetch-pack.c\n    +++ b/fetch-pack.c\n    @@ -1032,7 +1032,7 @@ static int get_pack(struct fetch_pack_args *args,\n            cmd.git_cmd = 1;\n            if (start_command(&cmd))\n                    die(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n    -       if (do_keep && (pack_lockfiles || fsck_objects)) {\n    +       if (do_keep && (pack_lockfiles /*|| fsck_objects*/)) {\n                    int is_well_formed;\n                    char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);\n\nmakes the crash go away, but I did not investigated what should be the\nproper logic.\n\nThanks beforehand for fixing this NULL-pointer dereference.\n\nKirill\n\n[1] https://lab.nexedi.com/kirr/git-backup\n[2] https://lab.nexedi.com/kirr/git-backup/-/blob/3230197c/git-backup.go#L717-739\n\nCc: Jonathan Tan <jonathantanmy@google.com>\nCc: Elijah Newren <newren@gmail.com>\nCc: Jeff King <peff@peff.net>\nCc: Calvin Wan <calvinwan@google.com>\nCc: Patrick Steinhardt <ps@pks.im>\nCc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nCc: Alain Takoudjou <alain.takoudjou@nexedi.com>\nCc: Jérome Perrin <jerome@nexedi.com>\nSigned-off-by: Kirill Smelkov <kirr@nexedi.com>\n---\n t/t5500-fetch-pack.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 1bc15a3f08..e3dbe79613 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -1056,6 +1056,20 @@ test_expect_success 'fetch with --filter=blob:limit=0' '\n \tfetch_filter_blob_limit_zero server server\n '\n \n+test_expect_failure 'fetch with fsckObjects but without --lock-pack does not segfault' '\n+\trm -rf server client &&\n+\tgit init server &&\n+\ttest_commit -C server 1 &&\n+\n+\tgit init client &&\n+\t# unpackLimit=1 forces to keep received pack as pack instead of\n+\t# unpacking it to loose objects. The code is currently segfaulting when\n+\t# trying to index that kept pack.\n+\tgit -c fetch.fsckObjects=true -c fetch.unpackLimit=1 \\\n+\t    -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/main)\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.39.2\n\n---- 8< ----\n\n\nOn Thu, Jun 06, 2024 at 04:36:05PM +0300, Kirill Smelkov wrote:\n> When running git-backup[1] over lab.nexedi.com archive we noticed that\n> Git started to crash on fetch-pack operation[2] after Git upgrade. We\n> tracked this down to be a regression introduced by Git 2.31, more\n> specifically by commit 5476e1efded5 (fetch-pack: print and use dangling\n> .gitmodules), which started to index and lock packfiles on do_keep=y &&\n> fsck_objects=y even if pack_lockfiles=NULL, which leads to\n> NULL-dereference when trying to append an entry to that pack_lockfiles\n> stringlist.\n> \n> Please find attached a test that demonstrates this problem.\n> \n> When run with test_expect_failure changed to test_expect_success the test\n> crashes with\n> \n>     ./test-lib.sh: line 1063: 599675 Segmentation fault      (core dumped) git -c fetch.fsckObjects=true -c fetch.unpackLimit=1 -C client fetch-pack ../server $(git -C server rev-parse refs/heads/main)\n> \n> and the backtrace is\n> \n>     Program terminated with signal SIGSEGV, Segmentation fault.\n>     #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,\n>         string=0x558033b695f0 \".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep\") at string-list.c:218\n>     218             ALLOC_GROW(list->items, list->nr + 1, list->alloc);\n>     (gdb) bt\n>     #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,\n>         string=0x558033b695f0 \".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep\") at string-list.c:218\n>     #1  0x0000558032b5b83f in get_pack (args=0x7ffe680f3fa0, xd=0x7ffe680f4058, pack_lockfiles=0x0, index_pack_args=0x0,\n>         sought=0x558033b65e90, nr_sought=1, gitmodules_oids=0x558032e17b88 <fsck_options+72>) at fetch-pack.c:1042\n>     #2  0x0000558032b5e0b3 in do_fetch_pack_v2 (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, orig_ref=0x558033b67b90, sought=0x558033b65e90,\n>         nr_sought=1, shallows=0x7ffe680f3df0, si=0x7ffe680f3e10, pack_lockfiles=0x0) at fetch-pack.c:1781\n>     #3  0x0000558032b5ef3c in fetch_pack (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, ref=0x558033b67b90, sought=0x558033b65e90, nr_sought=1,\n>         shallow=0x7ffe680f3f80, pack_lockfiles=0x0, version=protocol_v2) at fetch-pack.c:2073\n>     #4  0x0000558032a08851 in cmd_fetch_pack (argc=3, argv=0x7ffe680f4500, prefix=0x0) at builtin/fetch-pack.c:242\n>     #5  0x00005580329b2be3 in run_builtin (p=0x558032e0bb30 <commands+1008>, argc=3, argv=0x7ffe680f4500) at git.c:474\n>     #6  0x00005580329b2ffe in handle_builtin (argc=3, argv=0x7ffe680f4500) at git.c:729\n>     #7  0x00005580329b3222 in run_argv (argcp=0x7ffe680f433c, argv=0x7ffe680f4330) at git.c:793\n>     #8  0x00005580329b3796 in cmd_main (argc=3, argv=0x7ffe680f4500) at git.c:928\n>     #9  0x0000558032ab9002 in main (argc=10, argv=0x7ffe680f44c8) at common-main.c:62\n> \n> A simple debug patch below\n> \n>     --- a/fetch-pack.c\n>     +++ b/fetch-pack.c\n>     @@ -1032,7 +1032,7 @@ static int get_pack(struct fetch_pack_args *args,\n>             cmd.git_cmd = 1;\n>             if (start_command(&cmd))\n>                     die(_(\"fetch-pack: unable to fork off %s\"), cmd_name);\n>     -       if (do_keep && (pack_lockfiles || fsck_objects)) {\n>     +       if (do_keep && (pack_lockfiles /*|| fsck_objects*/)) {\n>                     int is_well_formed;\n>                     char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);\n> \n> makes the crash go away, but I did not investigated what should be the\n> proper logic.\n> \n> Thanks beforehand for fixing this NULL-pointer dereference.\n> \n> Kirill\n> \n> [1] https://lab.nexedi.com/kirr/git-backup\n> [2] https://lab.nexedi.com/kirr/git-backup/-/blob/3230197c/git-backup.go#L717-739\n> \n> Cc: Jonathan Tan <jonathantanmy@google.com>\n> Cc: Alain Takoudjou <alain.takoudjou@nexedi.com>\n> Cc: Jérome Perrin <jerome@nexedi.com>\n> Signed-off-by: Kirill Smelkov <kirr@nexedi.com>\n> ---\n>  t/t5500-fetch-pack.sh | 14 ++++++++++++++\n>  1 file changed, 14 insertions(+)\n> \n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 1bc15a3f08..e3dbe79613 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -1056,6 +1056,20 @@ test_expect_success 'fetch with --filter=blob:limit=0' '\n>  \tfetch_filter_blob_limit_zero server server\n>  '\n>  \n> +test_expect_failure 'fetch with fsckObjects but without --lock-pack does not segfault' '\n> +\trm -rf server client &&\n> +\tgit init server &&\n> +\ttest_commit -C server 1 &&\n> +\n> +\tgit init client &&\n> +\t# unpackLimit=1 forces to keep received pack as pack instead of\n> +\t# unpacking it to loose objects. The code is currently segfaulting when\n> +\t# trying to index that kept pack.\n> +\tgit -c fetch.fsckObjects=true -c fetch.unpackLimit=1 \\\n> +\t    -C client fetch-pack ../server \\\n> +\t\t$(git -C server rev-parse refs/heads/main)\n> +'\n> +\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n>  start_httpd\n>  \n> -- \n> 2.39.2\n\n"},{"id":"497347","messageId":"20240619130256.GA228005@coredump.intra.peff.net","threadId":"61597","inReplyTo":"ZnJpJyLPAKlu82s8@deca.navytux.spb.ru","subject":"[PATCH] fetch-pack: fix segfault when fscking without --lock-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T13:02:56Z","receivedAt":"2024-06-19T13:02:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2024 at 05:14:21AM +0000, Kirill Smelkov wrote:\n\n> + newren, peff, calvinwan, ps, avarab\n\nIt's fine (and even encouraged) to re-post a topic which didn't get any\nattention the first time around. But please don't mass-cc unrelated\npeople like this. We can all read the list and see your re-post, and if\nwe don't respond it may be because we have other priorities.\n\n> +test_expect_failure 'fetch with fsckObjects but without --lock-pack does not segfault' '\n> +\trm -rf server client &&\n> +\tgit init server &&\n> +\ttest_commit -C server 1 &&\n> +\n> +\tgit init client &&\n> +\t# unpackLimit=1 forces to keep received pack as pack instead of\n> +\t# unpacking it to loose objects. The code is currently segfaulting when\n> +\t# trying to index that kept pack.\n> +\tgit -c fetch.fsckObjects=true -c fetch.unpackLimit=1 \\\n> +\t    -C client fetch-pack ../server \\\n> +\t\t$(git -C server rev-parse refs/heads/main)\n> +'\n\nThanks for providing a reproduction of the problem.\n\nI think we don't want to stick the test right here, as it is breaking up\ntwo related tests (though it is confusing because one uses http and the\nother doesn't, so there's some http setup in between them). Though\ncuriously, putting the \"rm -rf server\" here revealed a minor bug in\nthose other tests. Fixed here:\n\n  https://lore.kernel.org/git/20240619125255.GA346466@coredump.intra.peff.net\n\nI think it's a bug that fetch.unpackLimit causes index-pack to create a\nlockfile at all, but there's a more direct way to trigger the issue,\nwhich I've used in the patch below. I'll follow up with more details in\na reply to the patch.\n\n-- >8 --\nSubject: fetch-pack: fix segfault when fscking without --lock-pack\n\nThe fetch-pack internals have multiple options related to creating\n\".keep\" lock-files for the received pack:\n\n  - if args.lock_pack is set, then we tell index-pack to create a .keep\n    file. In the fetch-pack plumbing command, this is triggered by\n    passing \"-k\" twice.\n\n  - if the caller passes in a pack_lockfiles string list, then we use it\n    to record the path of the keep-file created by index-pack. We get\n    that name by reading the stdout of index-pack. In the fetch-pack\n    command, this is triggered by passing the (undocumented) --lock-pack\n    option; without it, we pass in a NULL string list.\n\nSo it's possible to ask index-pack to create the lock-file (using \"-k\n-k\") but not ask to record it (by avoiding \"--lock-pack\"). This worked\nfine until 5476e1efde (fetch-pack: print and use dangling .gitmodules,\n2021-02-22), but now it causes a segfault.\n\nBefore that commit, if pack_lockfiles was NULL, we wouldn't bother\nreading the output from index-pack at all. But since that commit,\nindex-pack may produce extra output if we asked it to fsck. So even if\nnobody cares about the lockfile path, we still need to read it to skip\nto the output we do care about.\n\nWe correctly check that we didn't get a NULL lockfile path (which can\nhappen if we did not ask it to create a .keep file at all), but we\nmissed the case where the lockfile path is not NULL (due to \"-k -k\") but\nthe pack_lockfiles string_list is NULL (because nobody passed\n\"--lock-pack\"), and segfault trying to add to the NULL string-list.\n\nWe can fix this by skipping the append to the string list when either\nthe value or the list is NULL. In that case we must also free the\nlockfile path to avoid leaking it when it's non-NULL.\n\nNobody noticed the bug for so long because the transport code used by\n\"git fetch\" always passes in a pack_lockfiles pointer, and remote-curl\n(the main user of the fetch-pack plumbing command) always passes\n--lock-pack.\n\nReported-by: Kirill Smelkov <kirr@nexedi.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fetch-pack.c          |  4 +++-\n t/t5500-fetch-pack.sh | 10 ++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex eba9e420ea..42f48fbc31 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1038,8 +1038,10 @@ static int get_pack(struct fetch_pack_args *args,\n \n \t\tif (!is_well_formed)\n \t\t\tdie(_(\"fetch-pack: invalid index-pack output\"));\n-\t\tif (pack_lockfile)\n+\t\tif (pack_lockfiles && pack_lockfile)\n \t\t\tstring_list_append_nodup(pack_lockfiles, pack_lockfile);\n+\t\telse\n+\t\t\tfree(pack_lockfile);\n \t\tparse_gitmodules_oids(cmd.out, gitmodules_oids);\n \t\tclose(cmd.out);\n \t}\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex b26f367620..585ea0ee16 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -993,6 +993,16 @@ test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n \t\t       fetch origin server_has both_have_2\n '\n \n+test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault' '\n+\trm -rf server client &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server one &&\n+\n+\ttest_create_repo client &&\n+\tgit -c fetch.fsckObjects=true \\\n+\t    -C client fetch-pack -k -k ../server HEAD\n+'\n+\n test_expect_success 'filtering by size' '\n \trm -rf server client &&\n \ttest_create_repo server &&\n-- \n2.45.2.949.g1c649f6aed\n\n"},{"id":"497349","messageId":"20240619132208.GA895873@coredump.intra.peff.net","threadId":"61597","inReplyTo":"20240619130256.GA228005@coredump.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T13:22:08Z","receivedAt":"2024-06-19T13:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:\n\n> I think it's a bug that fetch.unpackLimit causes index-pack to create a\n> lockfile at all, but there's a more direct way to trigger the issue,\n> which I've used in the patch below. I'll follow up with more details in\n> a reply to the patch.\n\nYour original test used transfer.unpackLimit. By itself that should just\ncause us to avoid calling unpack-objects, keeping the pack we got, but\n_not_ creating a .keep file. Likewise, if you pass one \"-k\" to\nfetch-pack, it should just keep the pack but without a .keep file\n(that's what the double \"-k -k\" does).\n\nBut both of these do trigger a .keep file, which seems wrong to me. The\ncaller has no idea that the .keep file was created, so it won't clean it\nup, and the pack will be in limbo forever.\n\nI tried bisecting and came up with fa74052922 (Always obtain fetch-pack\narguments from struct fetch_pack_args, 2007-09-19). Given the length of\ntime it's been this way, that makes me a little afraid to touch it. ;)\nBut I think in practice it is not really triggered because of what I\nwrote earlier:\n\n> Nobody noticed the bug for so long because the transport code used by\n> \"git fetch\" always passes in a pack_lockfiles pointer, and remote-curl\n> (the main user of the fetch-pack plumbing command) always passes\n> --lock-pack.\n\nThat is, we're always asking for a lock-file anyway.\n\nBut it could affect external users of the fetch-pack plumbing. I.e., the\nvery command that produced the segfault for you is probably leaving an\nunexpected .keep file in place.\n\n> So it's possible to ask index-pack to create the lock-file (using \"-k\n> -k\") but not ask to record it (by avoiding \"--lock-pack\"). This worked\n> fine until 5476e1efde (fetch-pack: print and use dangling .gitmodules,\n> 2021-02-22), but now it causes a segfault.\n> \n> Before that commit, if pack_lockfiles was NULL, we wouldn't bother\n> reading the output from index-pack at all. But since that commit,\n> index-pack may produce extra output if we asked it to fsck. So even if\n> nobody cares about the lockfile path, we still need to read it to skip\n> to the output we do care about.\n\nThere's another interesting fallout from 5476e1efde that I noticed here.\nBefore that commit, if you did not pass --lock-pack to fetch-pack, then\nwe would never bother reading stdout from index-pack, and it would go to\nthe caller's stdout! So doing:\n\n  git fetch-pack -k -k repo HEAD\n\nwould produce:\n\n  keep\t3282886e55735beb9a08b048394284b03bef8488\n\nor similar on stdout. Which makes me wonder if some callers depend on\nthat. Or if it is simply a bug, since it would be intermingled with\nfetch-pack's actual output. We still produce that output today, but if\nyou use fetch.fsckObjects, then we eat it while looking for the other\nfsck-related output.\n\n-Peff\n"},{"id":"497350","messageId":"20240619133522.GA942902@coredump.intra.peff.net","threadId":"61597","inReplyTo":"20240619132208.GA895873@coredump.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T13:35:22Z","receivedAt":"2024-06-19T13:35:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2024 at 09:22:08AM -0400, Jeff King wrote:\n\n> On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:\n> \n> > I think it's a bug that fetch.unpackLimit causes index-pack to create a\n> > lockfile at all, but there's a more direct way to trigger the issue,\n> > which I've used in the patch below. I'll follow up with more details in\n> > a reply to the patch.\n> \n> Your original test used transfer.unpackLimit. By itself that should just\n> cause us to avoid calling unpack-objects, keeping the pack we got, but\n> _not_ creating a .keep file. Likewise, if you pass one \"-k\" to\n> fetch-pack, it should just keep the pack but without a .keep file\n> (that's what the double \"-k -k\" does).\n> \n> But both of these do trigger a .keep file, which seems wrong to me. The\n> caller has no idea that the .keep file was created, so it won't clean it\n> up, and the pack will be in limbo forever.\n> \n> I tried bisecting and came up with fa74052922 (Always obtain fetch-pack\n> arguments from struct fetch_pack_args, 2007-09-19). Given the length of\n> time it's been this way, that makes me a little afraid to touch it. ;)\n\nEven before that commit, we'd trigger the lock of unpack_limit was set\nthere. I find all of this code really puzzling (which makes me even more\nhesitant to touch it). But I really don't understand why it is not just\nthis:\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 42f48fbc31..ed57b0fdac 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -971,7 +971,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\tstrvec_push(&cmd.args, \"-v\");\n \t\tif (args->use_thin_pack)\n \t\t\tstrvec_push(&cmd.args, \"--fix-thin\");\n-\t\tif ((do_keep || index_pack_args) && (args->lock_pack || unpack_limit))\n+\t\tif ((do_keep || index_pack_args) && args->lock_pack)\n \t\t\tadd_index_pack_keep_option(&cmd.args);\n \t\tif (!index_pack_args && args->check_self_contained_and_connected)\n \t\t\tstrvec_push(&cmd.args, \"--check-self-contained-and-connected\");\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 585ea0ee16..d6d6ea6281 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -1003,6 +1003,28 @@ test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault\n \t    -C client fetch-pack -k -k ../server HEAD\n '\n \n+test_expect_success 'fetch-pack -k does not create .keep file' '\n+\trm -rf server client &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server one &&\n+\n+\ttest_create_repo client &&\n+\tgit -C client fetch-pack -k ../server HEAD &&\n+\tfind client/.git/objects/pack -name \"*.keep\" >keep &&\n+\ttest_must_be_empty keep\n+'\n+\n+test_expect_success 'fetch-pack with unpackLimit does not create .keep file' '\n+\trm -rf server client &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server one &&\n+\n+\ttest_create_repo client &&\n+\tgit -c fetch.unpackLimit=1 -C client fetch-pack ../server HEAD &&\n+\tfind client/.git/objects/pack -name \"*.keep\" >keep &&\n+\ttest_must_be_empty keep\n+'\n+\n test_expect_success 'filtering by size' '\n \trm -rf server client &&\n \ttest_create_repo server &&\n"},{"id":"497370","messageId":"ZnMz4A2ATa-QLAnS@deca.navytux.spb.ru","threadId":"61597","inReplyTo":"20240619133522.GA942902@coredump.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2024-06-19T19:39:18Z","receivedAt":"2024-06-19T19:39:19Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:\n> On Wed, Jun 19, 2024 at 05:14:21AM +0000, Kirill Smelkov wrote:\n> \n> > + newren, peff, calvinwan, ps, avarab\n> \n> It's fine (and even encouraged) to re-post a topic which didn't get any\n> attention the first time around. But please don't mass-cc unrelated\n> people like this. We can all read the list and see your re-post, and if\n> we don't respond it may be because we have other priorities.\n\nJeff, thanks for feedback and for reminding me about netiquette here.\nFrankly I though that for some reason my report about segmentation fault\nwas classified as spam and did not get people attention due to that.\n\nI appologize if my reposting with extended cc caused any inconvenience.\n\n\n> > +test_expect_failure 'fetch with fsckObjects but without --lock-pack does not segfault' '\n> > +\trm -rf server client &&\n> > +\tgit init server &&\n> > +\ttest_commit -C server 1 &&\n> > +\n> > +\tgit init client &&\n> > +\t# unpackLimit=1 forces to keep received pack as pack instead of\n> > +\t# unpacking it to loose objects. The code is currently segfaulting when\n> > +\t# trying to index that kept pack.\n> > +\tgit -c fetch.fsckObjects=true -c fetch.unpackLimit=1 \\\n> > +\t    -C client fetch-pack ../server \\\n> > +\t\t$(git -C server rev-parse refs/heads/main)\n> > +'\n> \n> Thanks for providing a reproduction of the problem.\n> \n> I think we don't want to stick the test right here, as it is breaking up\n> two related tests (though it is confusing because one uses http and the\n> other doesn't, so there's some http setup in between them). Though\n> curiously, putting the \"rm -rf server\" here revealed a minor bug in\n> those other tests. Fixed here:\n> \n>   https://lore.kernel.org/git/20240619125255.GA346466@coredump.intra.peff.net\n> \n> I think it's a bug that fetch.unpackLimit causes index-pack to create a\n> lockfile at all, but there's a more direct way to trigger the issue,\n> which I've used in the patch below. I'll follow up with more details in\n> a reply to the patch.\n> \n> -- >8 --\n> Subject: fetch-pack: fix segfault when fscking without --lock-pack\n> ...\n\nThanks for coming up with the fix promptly and for sure I might have\nmissed something when placing and doing my test. Thanks for correcting\nthat and coming with a more direct way to reproduce the issue.\n\n\nOn Wed, Jun 19, 2024 at 09:22:08AM -0400, Jeff King wrote:\n> On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:\n> \n> > I think it's a bug that fetch.unpackLimit causes index-pack to create a\n> > lockfile at all, but there's a more direct way to trigger the issue,\n> > which I've used in the patch below. I'll follow up with more details in\n> > a reply to the patch.\n> \n> Your original test used transfer.unpackLimit. By itself that should just\n> cause us to avoid calling unpack-objects, keeping the pack we got, but\n> _not_ creating a .keep file. Likewise, if you pass one \"-k\" to\n> fetch-pack, it should just keep the pack but without a .keep file\n> (that's what the double \"-k -k\" does).\n> \n> But both of these do trigger a .keep file, which seems wrong to me. The\n> caller has no idea that the .keep file was created, so it won't clean it\n> up, and the pack will be in limbo forever.\n> \n> I tried bisecting and came up with fa74052922 (Always obtain fetch-pack\n> arguments from struct fetch_pack_args, 2007-09-19). Given the length of\n> time it's been this way, that makes me a little afraid to touch it. ;)\n> But I think in practice it is not really triggered because of what I\n> wrote earlier:\n> \n> > Nobody noticed the bug for so long because the transport code used by\n> > \"git fetch\" always passes in a pack_lockfiles pointer, and remote-curl\n> > (the main user of the fetch-pack plumbing command) always passes\n> > --lock-pack.\n> \n> That is, we're always asking for a lock-file anyway.\n> \n> But it could affect external users of the fetch-pack plumbing. I.e., the\n> very command that produced the segfault for you is probably leaving an\n> unexpected .keep file in place.\n\nThanks for noticing. Yes, indeed I use `git fetch-pack` and expect only\nthe pack to be fetched without remaining it in pinned state via .keep file.\n\nIt would be good if that could be fixed because present behaviour\ncontradicts fetch-pack documentation that says that the pack is locked\nagainst being repacked only on double --keep.\n\n\n> > So it's possible to ask index-pack to create the lock-file (using \"-k\n> > -k\") but not ask to record it (by avoiding \"--lock-pack\"). This worked\n> > fine until 5476e1efde (fetch-pack: print and use dangling .gitmodules,\n> > 2021-02-22), but now it causes a segfault.\n> > \n> > Before that commit, if pack_lockfiles was NULL, we wouldn't bother\n> > reading the output from index-pack at all. But since that commit,\n> > index-pack may produce extra output if we asked it to fsck. So even if\n> > nobody cares about the lockfile path, we still need to read it to skip\n> > to the output we do care about.\n> \n> There's another interesting fallout from 5476e1efde that I noticed here.\n> Before that commit, if you did not pass --lock-pack to fetch-pack, then\n> we would never bother reading stdout from index-pack, and it would go to\n> the caller's stdout! So doing:\n> \n>   git fetch-pack -k -k repo HEAD\n> \n> would produce:\n> \n>   keep\t3282886e55735beb9a08b048394284b03bef8488\n> \n> or similar on stdout. Which makes me wonder if some callers depend on\n> that. Or if it is simply a bug, since it would be intermingled with\n> fetch-pack's actual output. We still produce that output today, but if\n> you use fetch.fsckObjects, then we eat it while looking for the other\n> fsck-related output.\n\nThanks for noticing as well. Indeed that lines emitted with keep might\nbe wanted to consume by a plumbing user. I'm not consuming them myself,\nbut from the output it looks like to be intended. Maybe almost noone\ndepend on those this days though.\n\nFor the reference: on my side I switched to fetch-pack plumbing from\nfetch porcelain because `git fetch` was working really slow when\ninitially checking whether it already locally has all the objects\nadvertised by remote:\n\nhttps://lab.nexedi.com/kirr/git-backup/commit/899103bf\nhttps://lab.nexedi.com/kirr/git-backup/commit/3efed898\n\n\nOn Wed, Jun 19, 2024 at 09:35:22AM -0400, Jeff King wrote:\n> On Wed, Jun 19, 2024 at 09:22:08AM -0400, Jeff King wrote:\n> \n> > On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:\n> > \n> > > I think it's a bug that fetch.unpackLimit causes index-pack to create a\n> > > lockfile at all, but there's a more direct way to trigger the issue,\n> > > which I've used in the patch below. I'll follow up with more details in\n> > > a reply to the patch.\n> > \n> > Your original test used transfer.unpackLimit. By itself that should just\n> > cause us to avoid calling unpack-objects, keeping the pack we got, but\n> > _not_ creating a .keep file. Likewise, if you pass one \"-k\" to\n> > fetch-pack, it should just keep the pack but without a .keep file\n> > (that's what the double \"-k -k\" does).\n> > \n> > But both of these do trigger a .keep file, which seems wrong to me. The\n> > caller has no idea that the .keep file was created, so it won't clean it\n> > up, and the pack will be in limbo forever.\n> > \n> > I tried bisecting and came up with fa74052922 (Always obtain fetch-pack\n> > arguments from struct fetch_pack_args, 2007-09-19). Given the length of\n> > time it's been this way, that makes me a little afraid to touch it. ;)\n> \n> Even before that commit, we'd trigger the lock of unpack_limit was set\n> there. I find all of this code really puzzling (which makes me even more\n> hesitant to touch it). But I really don't understand why it is not just\n> this:\n> \n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 42f48fbc31..ed57b0fdac 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -971,7 +971,7 @@ static int get_pack(struct fetch_pack_args *args,\n>  \t\t\tstrvec_push(&cmd.args, \"-v\");\n>  \t\tif (args->use_thin_pack)\n>  \t\t\tstrvec_push(&cmd.args, \"--fix-thin\");\n> -\t\tif ((do_keep || index_pack_args) && (args->lock_pack || unpack_limit))\n> +\t\tif ((do_keep || index_pack_args) && args->lock_pack)\n>  \t\t\tadd_index_pack_keep_option(&cmd.args);\n>  \t\tif (!index_pack_args && args->check_self_contained_and_connected)\n>  \t\t\tstrvec_push(&cmd.args, \"--check-self-contained-and-connected\");\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 585ea0ee16..d6d6ea6281 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -1003,6 +1003,28 @@ test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault\n>  \t    -C client fetch-pack -k -k ../server HEAD\n>  '\n>  \n> +test_expect_success 'fetch-pack -k does not create .keep file' '\n> +\trm -rf server client &&\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server one &&\n> +\n> +\ttest_create_repo client &&\n> +\tgit -C client fetch-pack -k ../server HEAD &&\n> +\tfind client/.git/objects/pack -name \"*.keep\" >keep &&\n> +\ttest_must_be_empty keep\n> +'\n> +\n> +test_expect_success 'fetch-pack with unpackLimit does not create .keep file' '\n> +\trm -rf server client &&\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server one &&\n> +\n> +\ttest_create_repo client &&\n> +\tgit -c fetch.unpackLimit=1 -C client fetch-pack ../server HEAD &&\n> +\tfind client/.git/objects/pack -name \"*.keep\" >keep &&\n> +\ttest_must_be_empty keep\n> +'\n> +\n>  test_expect_success 'filtering by size' '\n>  \trm -rf server client &&\n>  \ttest_create_repo server &&\n\nI once worked on Git internals in 72441af7c4e3 and around, but that was\nfor tree-diff and today I focus primarily on other areas. So I trust\nyour best judgement, and best judgement of people who are currently\nactive in Git development, to see whether it is ok or not.\n\nOn my side, as a user and primarily looking at the tests, it looks like\nto be a good thing to do not to create an unexpected .keep file.\n\nThanks, once again, for your prompt reply, explanations and for the fixes.\n\nKirill\n"},{"id":"497418","messageId":"xmqq4j9nwl82.fsf@gitster.g","threadId":"61597","inReplyTo":"20240619130256.GA228005@coredump.intra.peff.net","subject":"Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T17:57:33Z","receivedAt":"2024-06-20T17:57:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Before that commit, if pack_lockfiles was NULL, we wouldn't bother\n> reading the output from index-pack at all. But since that commit,\n> index-pack may produce extra output if we asked it to fsck. So even if\n> nobody cares about the lockfile path, we still need to read it to skip\n> to the output we do care about.\n\nTrue.  Looking at that problematic commit, I wonder how it passed\nthe review process.  As a design, adding a list of bare object IDs\nwithout marking what they are for is way too inextensible by our\nstandard practice.\n\nIt is probably not too late to fix it, as this is purely an internal\nimplementation detail between fetch-pack and index-pack that is not\neven documented (\"git index-pack --help\" does talk about the\n\"(pack|keep)\\t<pack-name>\" output, but never the output after that).\n\n> We correctly check that we didn't get a NULL lockfile path (which can\n> happen if we did not ask it to create a .keep file at all), but we\n> missed the case where the lockfile path is not NULL (due to \"-k -k\") but\n> the pack_lockfiles string_list is NULL (because nobody passed\n> \"--lock-pack\"), and segfault trying to add to the NULL string-list.\n>\n> We can fix this by skipping the append to the string list when either\n> the value or the list is NULL. In that case we must also free the\n> lockfile path to avoid leaking it when it's non-NULL.\n\nOK.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index eba9e420ea..42f48fbc31 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1038,8 +1038,10 @@ static int get_pack(struct fetch_pack_args *args,\n>  \n>  \t\tif (!is_well_formed)\n>  \t\t\tdie(_(\"fetch-pack: invalid index-pack output\"));\n> -\t\tif (pack_lockfile)\n> +\t\tif (pack_lockfiles && pack_lockfile)\n>  \t\t\tstring_list_append_nodup(pack_lockfiles, pack_lockfile);\n> +\t\telse\n> +\t\t\tfree(pack_lockfile);\n>  \t\tparse_gitmodules_oids(cmd.out, gitmodules_oids);\n>  \t\tclose(cmd.out);\n>  \t}\n\nThat looks like a very safe thing to do.\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index b26f367620..585ea0ee16 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -993,6 +993,16 @@ test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '\n>  \t\t       fetch origin server_has both_have_2\n>  '\n>  \n> +test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault' '\n> +\trm -rf server client &&\n> +\ttest_create_repo server &&\n> +\ttest_commit -C server one &&\n> +\n> +\ttest_create_repo client &&\n> +\tgit -c fetch.fsckObjects=true \\\n> +\t    -C client fetch-pack -k -k ../server HEAD\n> +'\n> +\n\nAnd the test is quite straight-forward.\n\nWill queue.  Thanks.\n"}]}