[RESEND,BUG,SIGSEGV,CRASH] (Re: [PATCH] fetch-pack: test: demonstrate segmentation fault when run with fsckObjects but without --lock-pack)

Commit Message

Kirill Smelkov June 19, 2024, 5:14 a.m. UTC

+ newren, peff, calvinwan, ps, avarab

Hello everyone. This patch demonstrates *segmentation fault* crash of Git
with, hopefully, properly written test. It was originally posted two
weeks ago without getting any replies nor any traces in seen/todo:

https://lore.kernel.org/git/[e-mail]/T/#m9d454aadc8857b84f17d1a331739f7399cf1bbf8

I understand that there might be something wrong with my test, but could
you please at least provide some feedback and acknowledge the problem?

Resending the patch and adding more people to Cc who touched fetch-pack.c recently.

Thanks beforehand for feedback,
Kirill

---- 8< ----
From: Kirill Smelkov <[e-mail]>
Date: Thu, 6 Jun 2024 16:03:44 +0300
Subject: [PATCH] fetch-pack: test: demonstrate segmentation fault when run
 with fsckObjects but without --lock-pack
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

When running git-backup[1] over lab.nexedi.com archive we noticed that
Git started to crash on fetch-pack operation[2] after Git upgrade. We
tracked this down to be a regression introduced by Git 2.31, more
specifically by commit 5476e1efded5 (fetch-pack: print and use dangling
.gitmodules), which started to index and lock packfiles on do_keep=y &&
fsck_objects=y even if pack_lockfiles=NULL, which leads to
NULL-dereference when trying to append an entry to that pack_lockfiles
stringlist.

Please find attached a test that demonstrates this problem.

When run with test_expect_failure changed to test_expect_success the test
crashes with

    ./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)

and the backtrace is

    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,
        string=0x558033b695f0 ".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep") at string-list.c:218
    218             ALLOC_GROW(list->items, list->nr + 1, list->alloc);
    (gdb) bt
    #0  0x0000558032c7ef3b in string_list_append_nodup (list=0x0,
        string=0x558033b695f0 ".git/objects/pack/pack-c1b2455a361bb50a0db087e7202db73d62938fa1.keep") at string-list.c:218
    #1  0x0000558032b5b83f in get_pack (args=0x7ffe680f3fa0, xd=0x7ffe680f4058, pack_lockfiles=0x0, index_pack_args=0x0,
        sought=0x558033b65e90, nr_sought=1, gitmodules_oids=0x558032e17b88 <fsck_options+72>) at fetch-pack.c:1042
    #2  0x0000558032b5e0b3 in do_fetch_pack_v2 (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, orig_ref=0x558033b67b90, sought=0x558033b65e90,
        nr_sought=1, shallows=0x7ffe680f3df0, si=0x7ffe680f3e10, pack_lockfiles=0x0) at fetch-pack.c:1781
    #3  0x0000558032b5ef3c in fetch_pack (args=0x7ffe680f3fa0, fd=0x7ffe680f4058, ref=0x558033b67b90, sought=0x558033b65e90, nr_sought=1,
        shallow=0x7ffe680f3f80, pack_lockfiles=0x0, version=protocol_v2) at fetch-pack.c:2073
    #4  0x0000558032a08851 in cmd_fetch_pack (argc=3, argv=0x7ffe680f4500, prefix=0x0) at builtin/fetch-pack.c:242
    #5  0x00005580329b2be3 in run_builtin (p=0x558032e0bb30 <commands+1008>, argc=3, argv=0x7ffe680f4500) at git.c:474
    #6  0x00005580329b2ffe in handle_builtin (argc=3, argv=0x7ffe680f4500) at git.c:729
    #7  0x00005580329b3222 in run_argv (argcp=0x7ffe680f433c, argv=0x7ffe680f4330) at git.c:793
    #8  0x00005580329b3796 in cmd_main (argc=3, argv=0x7ffe680f4500) at git.c:928
    #9  0x0000558032ab9002 in main (argc=10, argv=0x7ffe680f44c8) at common-main.c:62

A simple debug patch below

    --- a/fetch-pack.c
    +++ b/fetch-pack.c
    @@ -1032,7 +1032,7 @@ static int get_pack(struct fetch_pack_args *args,
            cmd.git_cmd = 1;
            if (start_command(&cmd))
                    die(_("fetch-pack: unable to fork off %s"), cmd_name);
    -       if (do_keep && (pack_lockfiles || fsck_objects)) {
    +       if (do_keep && (pack_lockfiles /*|| fsck_objects*/)) {
                    int is_well_formed;
                    char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);

makes the crash go away, but I did not investigated what should be the
proper logic.

Thanks beforehand for fixing this NULL-pointer dereference.

Kirill

[1] https://lab.nexedi.com/kirr/git-backup
[2] https://lab.nexedi.com/kirr/git-backup/-/blob/3230197c/git-backup.go#L717-739

Cc: Jonathan Tan <[e-mail]>
Cc: Elijah Newren <[e-mail]>
Cc: Jeff King <[e-mail]>
Cc: Calvin Wan <[e-mail]>
Cc: Patrick Steinhardt <[e-mail]>
Cc: Ævar Arnfjörð Bjarmason <[e-mail]>
Cc: Alain Takoudjou <[e-mail]>
Cc: Jérome Perrin <[e-mail]>
Signed-off-by: Kirill Smelkov <[e-mail]>
---
 t/t5500-fetch-pack.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Patch

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 1bc15a3f08..e3dbe79613 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -1056,6 +1056,20 @@  test_expect_success 'fetch with --filter=blob:limit=0' '
 	fetch_filter_blob_limit_zero server server
 '
 
+test_expect_failure 'fetch with fsckObjects but without --lock-pack does not segfault' '
+	rm -rf server client &&
+	git init server &&
+	test_commit -C server 1 &&
+
+	git init client &&
+	# unpackLimit=1 forces to keep received pack as pack instead of
+	# unpacking it to loose objects. The code is currently segfaulting when
+	# trying to index that kept pack.
+	git -c fetch.fsckObjects=true -c fetch.unpackLimit=1 \
+	    -C client fetch-pack ../server \
+		$(git -C server rev-parse refs/heads/main)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd