+ 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/20240606133605.602276-1-kirr@xxxxxxxxxx/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 <kirr@xxxxxxxxxx> 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 <jonathantanmy@xxxxxxxxxx> Cc: Elijah Newren <newren@xxxxxxxxx> Cc: Jeff King <peff@xxxxxxxx> Cc: Calvin Wan <calvinwan@xxxxxxxxxx> Cc: Patrick Steinhardt <ps@xxxxxx> Cc: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> Cc: Alain Takoudjou <alain.takoudjou@xxxxxxxxxx> Cc: Jérome Perrin <jerome@xxxxxxxxxx> Signed-off-by: Kirill Smelkov <kirr@xxxxxxxxxx> --- t/t5500-fetch-pack.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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 -- 2.39.2 ---- 8< ---- On Thu, Jun 06, 2024 at 04:36:05PM +0300, Kirill Smelkov wrote: > 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 <jonathantanmy@xxxxxxxxxx> > Cc: Alain Takoudjou <alain.takoudjou@xxxxxxxxxx> > Cc: Jérome Perrin <jerome@xxxxxxxxxx> > Signed-off-by: Kirill Smelkov <kirr@xxxxxxxxxx> > --- > t/t5500-fetch-pack.sh | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > 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 > > -- > 2.39.2
