# Bugreport: pack-objects died of signal 11 15 messages from 2022-07-12 to 2022-08-09. Participants: Paul Horn, Jeff King, Emily Noneman, Derrick Stolee, Ævar Arnfjörð Bjarmason, Junio C Hamano. Thread: https://gitlist.dev/t/58140 ## Paul Horn, 2022-07-12 08:51 Subject: Bugreport: pack-objects died of signal 11 Message-ID: <977a6e0e-251c-4fa5-8b3b-fa3e2a761926@www.fastmail.com> URL: https://gitlist.dev/e/977a6e0e-251c-4fa5-8b3b-fa3e2a761926%40www.fastmail.com ``` Hi, ultimately, `git repack` segfaults: $ git repack error: pack-objects died of signal 11 This also errors out GC runs and breaks index maintenance, though other commands still succeed. $ cat $(git rev-parse --git-dir)/gc.log error: pack-objects died of signal 11 fatal: failed to run repack The repository is a bare repository with 11 worktrees. I can only observe this error in this repository, others do not reproduce it. The attached log contains the output of `GIT_TRACE=1 GIT_TRACE_PACK_ACCESS=1 git repack` [System Info] git version: git version 2.37.0 cpu: arm64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Darwin 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000 arm64 compiler info: clang: 13.1.6 (clang-1316.0.21.2.5) libc info: no libc information available $SHELL (typically, interactive shell): /opt/homebrew/bin/bash [Enabled Hooks] Cheers, Paul ``` ## Jeff King, 2022-07-14 21:37 Subject: Re: Bugreport: pack-objects died of signal 11 Message-ID: URL: https://gitlist.dev/e/YtCMklbIoTAN%2FWRs%40coredump.intra.peff.net In-Reply-To: <977a6e0e-251c-4fa5-8b3b-fa3e2a761926@www.fastmail.com> ``` On Tue, Jul 12, 2022 at 10:51:29AM +0200, Paul Horn wrote: > ultimately, `git repack` segfaults: > > $ git repack > error: pack-objects died of signal 11 Can you provide a backtrace that shows where we segfaulted? If you can build Git from source and have access to a debugger like gdb, that should be enough. One option is to make sure core dumps are enabled (probably "ulimit -c unlimited"), then run the failing repack, then find the core file (usually in the current directory, but I'm not sure offhand about macOS), and then open the debugger on it ("gdb /path/to/your/git /path/to/core"). But perhaps easier may be to directly run the buggy pack-objects invocation. I pulled the options here from your trace file. Something like: gdb -ex 'set args pack-objects \ --delta-base-offset tmp-pack \ --keep-true-parents --honor-pack-keep --non-empty \ --all --reflog --indexed-objects --unpacked --incremental \ URL: https://gitlist.dev/e/ecb6fa27-b918-4234-8e44-13c2a3e76e07%40gmail.com In-Reply-To: ``` I am seeing a similar issue with git on macos. git repack fails with signal 11. Other git commands seem to be working fine. My version info: [System Info] git version: git version 2.37.1.311.g350dc9f0e8 cpu: x86_64 built from commit: 350dc9f0e8974b6fcbdeb3808186c5a79c3e7386 sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64 compiler info: clang: 13.1.6 (clang-1316.0.21.2.5) libc info: no libc information available $SHELL (typically, interactive shell): /bin/zsh [Enabled Hooks] The stack trace: Thread 2 received signal SIGSEGV, Segmentation fault. git_config_check_init (repo=0x0) at config.c:2538 2538        if (repo->config && repo->config->hash_initialized) (gdb) bt #0  git_config_check_init (repo=0x0) at config.c:2538 #1  0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 "status.showuntrackedfiles", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574 #2  0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781 #3  new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797 #4  0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996 #5  post_read_index_from (istate=0x7ff7bfefc7e0, istate@entry=0x600003b02920) at read-cache.c:2028 #6  0x00000001001d6599 in read_index_from (istate=0x600003b02920, istate@entry=0x0, path=path@entry=0xffffffff ,     gitdir=0x600002c08080 "/Users/emily.noneman/workspace/webdev/.NetLedger_LocalBranchData.git") at read-cache.c:2457 #7  0x00000001001fcb2d in add_index_objects_to_pending (revs=0x600003b02920, revs@entry=0x7ff7bfefe9a0, flags=flags@entry=0) at revision.c:1783 #8  0x00000001002001e4 in handle_revision_pseudo_opt (revs=0x7ff7bfefe9a0, argv=0x600003700020, flags=) at revision.c:2717 #9  setup_revisions (argc=61876512, argv=0x600003700000, revs=, revs@entry=0x1002a3c49, opt=, opt@entry=0x7ff7bfefca50) at revision.c:2806 #10 0x0000000100084092 in get_object_list (revs=0x1002a3c49, revs@entry=0x0, ac=0, av=0x1002a3c49) at builtin/pack-objects.c:3993 #11 0x0000000100080a07 in cmd_pack_objects (argc=, argv=, prefix=) at builtin/pack-objects.c:4466 #12 0x0000000100005313 in run_builtin (p=0x600003b02920, p@entry=0xffffffff, argc=11, argc@entry=0, argv=, argv@entry=0x1002a3c49) at git.c:466 #13 0x0000000100004566 in handle_builtin (argc=0, argv=0x1002a3c49) at git.c:720 #14 0x0000000100003b59 in run_argv (argcp=0x7ff7bfeff7a4, argv=0x7ff7bfeff798) at git.c:787 #15 cmd_main (argc=11, argc@entry=0, argv=0x7ff7bfeff968, argv@entry=0x1002a3c49) at git.c:920 #16 0x00000001000d49a5 in main (argc=0, argv=0x1002a3c49) at common-main.c:56 On 7/14/22 5:37 PM, Jeff King wrote: > On Tue, Jul 12, 2022 at 10:51:29AM +0200, Paul Horn wrote: > >> ultimately, `git repack` segfaults: >> >> $ git repack >> error: pack-objects died of signal 11 > Can you provide a backtrace that shows where we segfaulted? If you can > build Git from source and have access to a debugger like gdb, that > should be enough. > > One option is to make sure core dumps are enabled (probably "ulimit -c > unlimited"), then run the failing repack, then find the core file > (usually in the current directory, but I'm not sure offhand about > macOS), and then open the debugger on it ("gdb /path/to/your/git > /path/to/core"). > > But perhaps easier may be to directly run the buggy pack-objects > invocation. I pulled the options here from your trace file. Something > like: > > gdb -ex 'set args pack-objects \ > --delta-base-offset tmp-pack \ > --keep-true-parents --honor-pack-keep --non-empty \ > --all --reflog --indexed-objects --unpacked --incremental \ > /path/to/your/git > > Then "r" to run inside the debugger, which will stop if we segfault. And > then "bt" will show the backtrace. > > Alternatively, is it possible to make the offending repository available > (even off-list)? > > -Peff > > ``` ## Derrick Stolee, 2022-08-04 20:14 Subject: Re: Bugreport: pack-objects died of signal 11 Message-ID: <87d46db2-8e09-e5a7-b6bb-e94bf05df305@github.com> URL: https://gitlist.dev/e/87d46db2-8e09-e5a7-b6bb-e94bf05df305%40github.com In-Reply-To: ``` On 8/4/2022 2:21 PM, Emily Noneman wrote: > The stack trace: > Thread 2 received signal SIGSEGV, Segmentation fault. > git_config_check_init (repo=0x0) at config.c:2538 > 2538        if (repo->config && repo->config->hash_initialized) > (gdb) bt > #0  git_config_check_init (repo=0x0) at config.c:2538 > #1  0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 "status.showuntrackedfiles", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574 > #2  0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781 > #3  new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797 > #4  0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996 Here is where things are confusing: * tweak_untracked_cache() takes an 'istate' that is non-NULL here. * The next spot in the stack is new_untracked_cache() with a NULL 'istate'. The only way these are connected is by a missing stack frame (probably optimized out) calling add_untracked_cache(). Still, it should be passing 'istate' throughout this process. The repo_config_get_string() call must also be coming from new_untracked_cache_flags() which is again a missing stack frame, but is called from new_untracked_cache(). Strangely, it's using a NULL 'repo' here which should have come from 'istate->repo', so we should have had a segfault earlier. Sorry for the drive-by commentary without any solution. This is just genuinely puzzling to me. Thanks, -Stolee ``` ## Ævar Arnfjörð Bjarmason, 2022-08-04 21:07 Subject: Re: Bugreport: pack-objects died of signal 11 Message-ID: <220804.86tu6rso6n.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/220804.86tu6rso6n.gmgdl%40evledraar.gmail.com In-Reply-To: <87d46db2-8e09-e5a7-b6bb-e94bf05df305@github.com> ``` On Thu, Aug 04 2022, Derrick Stolee wrote: > On 8/4/2022 2:21 PM, Emily Noneman wrote: >> The stack trace: >> Thread 2 received signal SIGSEGV, Segmentation fault. >> git_config_check_init (repo=0x0) at config.c:2538 >> 2538        if (repo->config && repo->config->hash_initialized) >> (gdb) bt >> #0  git_config_check_init (repo=0x0) at config.c:2538 >> #1  0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 "status.showuntrackedfiles", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574 >> #2  0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781 >> #3  new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797 >> #4  0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996 > > Here is where things are confusing: > > * tweak_untracked_cache() takes an 'istate' that is non-NULL here. > > * The next spot in the stack is new_untracked_cache() with a NULL 'istate'. > > The only way these are connected is by a missing stack frame (probably > optimized out) calling add_untracked_cache(). Still, it should be > passing 'istate' throughout this process. > > The repo_config_get_string() call must also be coming from > new_untracked_cache_flags() which is again a missing stack frame, > but is called from new_untracked_cache(). Strangely, it's using > a NULL 'repo' here which should have come from 'istate->repo', so > we should have had a segfault earlier. > > Sorry for the drive-by commentary without any solution. This is > just genuinely puzzling to me. I think this segfault might be fixed by this patch of mine, which I wrote for something unrelated back in April (but it was never sent to the list). https://github.com/avar/git/commit/d83bfa866ba Emily and/or Paul: Are you able to test the patch to see if it would work, diff here: https://github.com/avar/git/commit/d83bfa866ba.patch It's exactly on the codepath in this stacktrace, i.e. add_index_objects_to_pending() in revision.c will do before/after: - struct index_state istate = { NULL }; + struct index_state istate = { .repo = revs->repo }; Then when we're all the way down in new_untracked_cache_flags() we do: struct repository *repo = istate->repo; Which then calls (indirectly) git_config_check_init(), and we segfault not because istate is NULL, but because the "repo" it's carrying is NULL. But maybe I'm wrong, I haven't been able to reproduce this. The reason I wrote that patch (as can be seen if you peek at the WIP branch it's at) is because I ran into a similar dependency between the_index and the_repo & an istate variable being passed around with fsmonitor-settings.c. ``` ## Emily Noneman, 2022-08-04 22:32 Subject: Re: Bugreport: pack-objects died of signal 11 Message-ID: URL: https://gitlist.dev/e/CAKvVO18RVye%3DPkBRv%3Dtrj2GHh8ccGKL5j0mMq2eHQ1SX%3Dwsr8A%40mail.gmail.com In-Reply-To: <220804.86tu6rso6n.gmgdl@evledraar.gmail.com> ``` I have an updated backtrace with optimizations turned off: Thread 2 received signal SIGSEGV, Segmentation fault. 0x000000010016f770 in git_config_check_init (repo=0x0) at config.c:2538 2538 if (repo->config && repo->config->hash_initialized) (gdb) bt #0 0x000000010016f770 in git_config_check_init (repo=0x0) at config.c:2538 #1 0x000000010016f94d in repo_config_get_string (repo=0x0, key=0x10036e541 "status.showuntrackedfiles", dest=0x7ff7bfefc6d0) at config.c:2574 #2 0x00000001001b4dbf in new_untracked_cache_flags (istate=0x7ff7bfefc858) at dir.c:2781 #3 0x00000001001b0027 in new_untracked_cache (istate=0x7ff7bfefc858, flags=-1) at dir.c:2797 #4 0x00000001001aff6c in add_untracked_cache (istate=0x7ff7bfefc858) at dir.c:2806 #5 0x0000000100276126 in tweak_untracked_cache (istate=0x7ff7bfefc858) at read-cache.c:1996 #6 0x000000010027352e in post_read_index_from (istate=0x7ff7bfefc858) at read-cache.c:2028 #7 0x0000000100273228 in read_index_from (istate=0x7ff7bfefc858, path=0x600002c0ac00 "/Users/emily.noneman/workspace/webdev/.NetLedger_LocalBranchData.git/index", gitdir=0x600002c04080 "/Users/emily.noneman/workspace/webdev/.NetLedger_LocalBranchData.git") at read-cache.c:2457 #8 0x00000001002a98c1 in add_index_objects_to_pending (revs=0x7ff7bfefcfe8, flags=0) at revision.c:1783 #9 0x00000001002adb4b in handle_revision_pseudo_opt (revs=0x7ff7bfefcfe8, argv=0x600003704020, flags=0x7ff7bfefcaf8) at revision.c:2717 #10 0x00000001002acd2c in setup_revisions (argc=6, argv=0x600003704000, revs=0x7ff7bfefcfe8, opt=0x7ff7bfefcb78) at revision.c:2806 #11 0x00000001000a747e in get_object_list (revs=0x7ff7bfefcfe8, ac=6, av=0x600003704000) at builtin/pack-objects.c:3993 #12 0x00000001000a5f30 in cmd_pack_objects (argc=0, argv=0x7ff7bfeff968, prefix=0x0) at builtin/pack-objects.c:4472 #13 0x0000000100002a03 in run_builtin (p=0x1003d0fb8 , argc=11, argv=0x7ff7bfeff968) at git.c:466 #14 0x0000000100001528 in handle_builtin (argc=11, argv=0x7ff7bfeff968) at git.c:720 #15 0x0000000100002406 in run_argv (argcp=0x7ff7bfeff7dc, argv=0x7ff7bfeff7d0) at git.c:787 #16 0x00000001000012f9 in cmd_main (argc=11, argv=0x7ff7bfeff968) at git.c:920 #17 0x00000001001155c6 in main (argc=12, argv=0x7ff7bfeff960) at common-main.c:56 Ævar Arnfjörð Bjarmason, your patch fixed the issue for me! Thank you! On Thu, Aug 4, 2022 at 5:18 PM Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Aug 04 2022, Derrick Stolee wrote: > > > On 8/4/2022 2:21 PM, Emily Noneman wrote: > >> The stack trace: > >> Thread 2 received signal SIGSEGV, Segmentation fault. > >> git_config_check_init (repo=0x0) at config.c:2538 > >> 2538 if (repo->config && repo->config->hash_initialized) > >> (gdb) bt > >> #0 git_config_check_init (repo=0x0) at config.c:2538 > >> #1 0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 "status.showuntrackedfiles", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574 > >> #2 0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781 > >> #3 new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797 > >> #4 0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996 > > > > Here is where things are confusing: > > > > * tweak_untracked_cache() takes an 'istate' that is non-NULL here. > > > > * The next spot in the stack is new_untracked_cache() with a NULL 'istate'. > > > > The only way these are connected is by a missing stack frame (probably > > optimized out) calling add_untracked_cache(). Still, it should be > > passing 'istate' throughout this process. > > > > The repo_config_get_string() call must also be coming from > > new_untracked_cache_flags() which is again a missing stack frame, > > but is called from new_untracked_cache(). Strangely, it's using > > a NULL 'repo' here which should have come from 'istate->repo', so > > we should have had a segfault earlier. > > > > Sorry for the drive-by commentary without any solution. This is > > just genuinely puzzling to me. > > I think this segfault might be fixed by this patch of mine, which I > wrote for something unrelated back in April (but it was never sent to > the list). > > https://github.com/avar/git/commit/d83bfa866ba > > Emily and/or Paul: Are you able to test the patch to see if it would > work, diff here: https://github.com/avar/git/commit/d83bfa866ba.patch > > It's exactly on the codepath in this stacktrace, > i.e. add_index_objects_to_pending() in revision.c will do before/after: > > - struct index_state istate = { NULL }; > + struct index_state istate = { .repo = revs->repo }; > > Then when we're all the way down in new_untracked_cache_flags() we do: > > struct repository *repo = istate->repo; > > Which then calls (indirectly) git_config_check_init(), and we segfault > not because istate is NULL, but because the "repo" it's carrying is > NULL. > > But maybe I'm wrong, I haven't been able to reproduce this. > > The reason I wrote that patch (as can be seen if you peek at the WIP > branch it's at) is because I ran into a similar dependency between > the_index and the_repo & an istate variable being passed around with > fsmonitor-settings.c. ``` ## Ævar Arnfjörð Bjarmason, 2022-08-05 14:24 Subject: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/patch-1.1-3ff17707481-20220805T141816Z-avarab%40gmail.com In-Reply-To: ``` Narrowly fix a segfault that's been reported against pack-objects at [1] and [2]. This fixes a segfault in "git pack-objects", see a reports at [1] and [2]. In that case we'd go through the add_index_objects_to_pending() codepath being modified here, and eventually call into new_untracked_cache(). We'd then segfault in new_untracked_cache_flags() as we tried to use a NULL "repo" extracted from the "struct index_state". The segfault we ran into there is only a symptom of a much bigger problem, namely that we have various "struct index_state" variables constructed in various places, and then call into functions that might expect to have a non-NULL "repo" member. In follow-up work we'll fix that more general problem, but let's just fix the segfault for now. 1. https://lore.kernel.org/git/977a6e0e-251c-4fa5-8b3b-fa3e2a761926@www.fastmail.com/ 2. https://lore.kernel.org/git/ecb6fa27-b918-4234-8e44-13c2a3e76e07@gmail.com/ Reported-by: Paul Horn Reported-by: Emily Noneman Signed-off-by: Ævar Arnfjörð Bjarmason --- On Thu, Aug 04 2022, Emily Noneman wrote: > Ævar Arnfjörð Bjarmason, your patch fixed the issue for me! Thank you! Thanks, would you mind testing this more narrow fix, I think it should fix the segfault too, but let's be sure & confirm it. I haven't (and haven't really tried) to come up with a regression fix for this. I have changes that come after this which extensively test the same issue elsewhere, so maybe it's not needed... revision.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/revision.c b/revision.c index 87f1c117d13..48c2f1b91a8 100644 --- a/revision.c +++ b/revision.c @@ -1775,7 +1775,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags) worktrees = get_worktrees(); for (p = worktrees; *p; p++) { struct worktree *wt = *p; - struct index_state istate = { NULL }; + struct index_state istate = { .repo = revs->repo }; if (wt->is_current) continue; /* current index already taken care of */ -- 2.37.1.1283.g2c56c4202c5 ``` ## Derrick Stolee, 2022-08-05 14:48 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/b6299f8a-f75a-0e96-a6a6-55a7280584bf%40github.com In-Reply-To: ``` On 8/5/2022 10:24 AM, Ævar Arnfjörð Bjarmason wrote: > Narrowly fix a segfault that's been reported against pack-objects at > [1] and [2]. > > This fixes a segfault in "git pack-objects", see a reports at [1] and > [2]. In that case we'd go through the add_index_objects_to_pending() > codepath being modified here, and eventually call into > new_untracked_cache(). > > We'd then segfault in new_untracked_cache_flags() as we tried to use a > NULL "repo" extracted from the "struct index_state". > struct worktree *wt = *p; > - struct index_state istate = { NULL }; > + struct index_state istate = { .repo = revs->repo }; Thanks for the fix. It is definitely narrow to this particular stack trace. It is rare that we create an index that is not 'the_index' in this way. However, there are similar { NULL } initializers that could be cleaned up in these files: * apply.c * merge-recursive.c * builtin/stash.c * builtin/worktree.c Of course, delay these for a follow-up since these uses are not causing segfaults in the wild. Thanks, -Stolee ``` ## Jeff King, 2022-08-05 15:25 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/Yu02dOo4G8sy8tI6%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Aug 05, 2022 at 10:48:43AM -0400, Derrick Stolee wrote: > On 8/5/2022 10:24 AM, Ævar Arnfjörð Bjarmason wrote: > > Narrowly fix a segfault that's been reported against pack-objects at > > [1] and [2]. > > > > This fixes a segfault in "git pack-objects", see a reports at [1] and > > [2]. In that case we'd go through the add_index_objects_to_pending() > > codepath being modified here, and eventually call into > > new_untracked_cache(). > > > > We'd then segfault in new_untracked_cache_flags() as we tried to use a > > NULL "repo" extracted from the "struct index_state". > > > struct worktree *wt = *p; > > - struct index_state istate = { NULL }; > > + struct index_state istate = { .repo = revs->repo }; > > Thanks for the fix. It is definitely narrow to this particular > stack trace. > > It is rare that we create an index that is not 'the_index' in > this way. However, there are similar { NULL } initializers that > could be cleaned up in these files: > > * apply.c > * merge-recursive.c > * builtin/stash.c > * builtin/worktree.c > > Of course, delay these for a follow-up since these uses are > not causing segfaults in the wild. We did have another similar segfault recently: https://lore.kernel.org/git/YtrdPguYs3a3xekv@kitenet.net/ It's not quite the same case, but I think there's some overlap, and the reason Ævar couldn't reproduce is that Martin's patch in that thread has already fixed this case, too. Using git v2.37.1, I can get a segfault with a backtrace similar to Emily's by running: git init repo cd repo git commit --allow-empty -m base git config core.untrackedCache true git worktree add at rm .git/worktrees/wt/index git gc And applying the patch from this thread does make it go away. But likewise for cherry-picking 4447d4129d (read-cache: make `do_read_index()` always set up `istate->repo`, 2022-07-22), which is already in "master". Of the two patches, I think 4447d4129d is the better approach. The assumption in the code seems to be that do_read_index() (and thus read_index_from(), etc) will set up istate->repo. That patch fixes a corner case where we failed to do so. And with that fix, there's no need for the callers to set things up ahead of time. So it covers all of those initializers you mentioned. Emily, Paul: I'm 99% sure this will be the case given my reproduction above, but if you could try reproducing the problem with the current tip of "master" from git.git, that would confirm the findings. -Peff ``` ## Junio C Hamano, 2022-08-05 16:41 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/xmqqo7wyzlpm.fsf%40gitster.g In-Reply-To: ``` Jeff King writes: > Of the two patches, I think 4447d4129d is the better approach. The > assumption in the code seems to be that do_read_index() (and thus > read_index_from(), etc) will set up istate->repo. That patch fixes a > corner case where we failed to do so. And with that fix, there's no need > for the callers to set things up ahead of time. So it covers all of > those initializers you mentioned. Yeah, I tend to agree that Martin's fix, which is a more focused one, is the better approach between the two. It was merged to the 'master' track only a few days ago. We are at the end of the week #4 of this 12-week cycle, and we've accumulated about two dozens of topics already on 'maint' since the first maintenance release v2.37.1 was done, so we may have a chance to merge Martin's fix down to 'maint' before tagging the v2.37.2 release. One thing that may help is to add a test similar to the situation Emily & Paul had to t7063 on top of 4447d4129d before merging it down, perhaps? In your reproduction, the "rm .git/index" step makes the worktree's branch "not checked out" (the commit is empty so "worktree add" may check out no files, and removing the index will make it as if you did "clone --no-checkout") ... git init repo cd repo git commit --allow-empty -m base git config core.untrackedCache true git worktree add at rm .git/worktrees/wt/index git gc ... but it is not something an end user is likely to do, so I am still curious how this was triggered in real life. Ah, OK, I think the steps can be tweaked to git config core.untrackedCache true - git worktree add at - rm .git/worktrees/wt/index + git worktree add --no-checkout wt git gc i.e. (1) With "worktree add --no-checkout", there is no need to manually remove the index file, and (2) "at" is an obvious typo of "wt". This does not require the history to be a singleton empty tree, either. And that is a less implausible thing for users to be doing, but may still not be very common. Perhaps the --no-checkout is a prelude to set up a custom sparse checkout pattern, to avoid a wasteful full tree checkout before doing so, or something? If that is the case, then the above sequence becomes a very plausible thing for users to be doing. > Emily, Paul: I'm 99% sure this will be the case given my reproduction > above, but if you could try reproducing the problem with the current tip > of "master" from git.git, that would confirm the findings. Yes, indded. That would be very helpful. Thanks, all, for discussing the problem and its solution(s), and thanks in advance for further testing to help the fix forward. ``` ## Emily Noneman, 2022-08-08 18:15 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/CAKvVO18Du5M6broAJ6WqTAk6C4jxtqbXoS%3D9x03JgSdJAak80Q%40mail.gmail.com In-Reply-To: ``` The latest changes seem to have resolved the issue for me. My repo was a bit different than Pauls, it's a big corporate repo with a partial checkout and a worktree. What's confusing is that I do see an "index" file in worktrees/NetLedger_GitRepo , but git 2.37.1 exhibits the crash. 2.37.1.377.g679aad9e82 works just fine. On Fri, Aug 5, 2022 at 12:42 PM Junio C Hamano wrote: > > Jeff King writes: > > > Of the two patches, I think 4447d4129d is the better approach. The > > assumption in the code seems to be that do_read_index() (and thus > > read_index_from(), etc) will set up istate->repo. That patch fixes a > > corner case where we failed to do so. And with that fix, there's no need > > for the callers to set things up ahead of time. So it covers all of > > those initializers you mentioned. > > Yeah, I tend to agree that Martin's fix, which is a more focused > one, is the better approach between the two. It was merged to > the 'master' track only a few days ago. > > We are at the end of the week #4 of this 12-week cycle, and we've > accumulated about two dozens of topics already on 'maint' since the > first maintenance release v2.37.1 was done, so we may have a chance > to merge Martin's fix down to 'maint' before tagging the v2.37.2 > release. > > One thing that may help is to add a test similar to the situation > Emily & Paul had to t7063 on top of 4447d4129d before merging it > down, perhaps? > > In your reproduction, the "rm .git/index" step makes the worktree's > branch "not checked out" (the commit is empty so "worktree add" may > check out no files, and removing the index will make it as if you > did "clone --no-checkout") ... > > git init repo > cd repo > git commit --allow-empty -m base > > git config core.untrackedCache true > git worktree add at > rm .git/worktrees/wt/index > > git gc > > ... but it is not something an end user is likely to do, so I am > still curious how this was triggered in real life. > > Ah, OK, I think the steps can be tweaked to > > git config core.untrackedCache true > - git worktree add at > - rm .git/worktrees/wt/index > + git worktree add --no-checkout wt > > git gc > > i.e. (1) With "worktree add --no-checkout", there is no need to > manually remove the index file, and (2) "at" is an obvious typo of > "wt". This does not require the history to be a singleton empty > tree, either. > > And that is a less implausible thing for users to be doing, but may > still not be very common. Perhaps the --no-checkout is a prelude to > set up a custom sparse checkout pattern, to avoid a wasteful full > tree checkout before doing so, or something? If that is the case, > then the above sequence becomes a very plausible thing for users to > be doing. > > > Emily, Paul: I'm 99% sure this will be the case given my reproduction > > above, but if you could try reproducing the problem with the current tip > > of "master" from git.git, that would confirm the findings. > > Yes, indded. That would be very helpful. > > Thanks, all, for discussing the problem and its solution(s), and > thanks in advance for further testing to help the fix forward. ``` ## Jeff King, 2022-08-09 12:54 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/YvJZCGyhd6KJCabi%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Aug 05, 2022 at 09:41:57AM -0700, Junio C Hamano wrote: > Ah, OK, I think the steps can be tweaked to > > git config core.untrackedCache true > - git worktree add at > - rm .git/worktrees/wt/index > + git worktree add --no-checkout wt > > git gc > > i.e. (1) With "worktree add --no-checkout", there is no need to > manually remove the index file, and (2) "at" is an obvious typo of > "wt". This does not require the history to be a singleton empty > tree, either. That makes sense. I worked backwards from the guess that Martin's patch was relevant, hence trying to trigger ENOENT on the file. Using --no-checkout is a plausible way to get there. (And yes, the "at" is a typo. I double-checked the script I actually ran and it did not have the typo; not sure how I managed to introduce it via cut and paste!). The singleton empty tree was only there in my example because "git worktree" insists on having a starting point. Its contents shouldn't matter either way. Here's the test addition you mentioned, though I'm really not sure it's adding much. The source lines for both the bug/fix and the spot where we segfault should be identical between the two cases. It's really just about how we ended up calling into the code. -- >8 -- Subject: [PATCH] t7063: add another missing-index test Commit 4447d4129d (read-cache: make `do_read_index()` always set up `istate->repo`, 2022-07-22) fixed a case where a missing "index" file caused us to segfault in the untracked-cache code. We ran into another situation in the wild which caused a similar segfault, this time with an empty worktree. These actually segfault at the same spot (inside the index reading code), but the call path to get there is different between the two, hence it may be worth checking both. Helped-by: Junio C Hamano Signed-off-by: Jeff King --- t/t7063-status-untracked-cache.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index c1f0d95036..a0e8920ffa 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -990,4 +990,9 @@ test_expect_success 'empty repo (no index) and core.untrackedCache' ' git -C emptyrepo -c core.untrackedCache=true write-tree ' +test_expect_success 'empty worktree and core.untrackedCache' ' + git worktree add --no-checkout empty-worktree && + git -c core.untrackedCache=true gc +' + test_done -- 2.37.1.845.ge5d2b690ea ``` ## Jeff King, 2022-08-09 12:55 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/YvJZWUVULB5Y8zih%40coredump.intra.peff.net In-Reply-To: ``` On Mon, Aug 08, 2022 at 02:15:21PM -0400, Emily Noneman wrote: > The latest changes seem to have resolved the issue for me. My repo was > a bit different than Pauls, it's a big corporate repo with a partial > checkout and a worktree. What's confusing is that I do see an "index" > file in worktrees/NetLedger_GitRepo , but git 2.37.1 exhibits the > crash. 2.37.1.377.g679aad9e82 works just fine. Thanks for testing. It is odd that you see an "index" file, since the code change is definitely focused around getting ENOENT from open() on the index. Is it possible that you have another worktree (even the initial one) without an index in it? -Peff ``` ## Junio C Hamano, 2022-08-09 16:55 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/xmqqedxpqrvc.fsf%40gitster.g In-Reply-To: ``` Jeff King writes: > Here's the test addition you mentioned, though I'm really not sure it's > adding much. The source lines for both the bug/fix and the spot where we > segfault should be identical between the two cases. It's really just > about how we ended up calling into the code. True, but the "non-existing index file" case is rare enough that it would be worth to document the new way to get into the state, I would think. I am offline every other Tuesday and today is such a day, so please expect no changes to the public repositories. Thanks. > t/t7063-status-untracked-cache.sh | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh > index c1f0d95036..a0e8920ffa 100755 > --- a/t/t7063-status-untracked-cache.sh > +++ b/t/t7063-status-untracked-cache.sh > @@ -990,4 +990,9 @@ test_expect_success 'empty repo (no index) and core.untrackedCache' ' > git -C emptyrepo -c core.untrackedCache=true write-tree > ' > > +test_expect_success 'empty worktree and core.untrackedCache' ' > + git worktree add --no-checkout empty-worktree && > + git -c core.untrackedCache=true gc > +' > + > test_done ``` ## Junio C Hamano, 2022-08-09 16:56 Subject: Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects Message-ID: URL: https://gitlist.dev/e/xmqqa68dqrtj.fsf%40gitster.g In-Reply-To: ``` Jeff King writes: > On Mon, Aug 08, 2022 at 02:15:21PM -0400, Emily Noneman wrote: > >> The latest changes seem to have resolved the issue for me. My repo was >> a bit different than Pauls, it's a big corporate repo with a partial >> checkout and a worktree. What's confusing is that I do see an "index" >> file in worktrees/NetLedger_GitRepo , but git 2.37.1 exhibits the >> crash. 2.37.1.377.g679aad9e82 works just fine. > > Thanks for testing. It is odd that you see an "index" file, since the > code change is definitely focused around getting ENOENT from open() on > the index. Is it possible that you have another worktree (even the > initial one) without an index in it? Yes, it affects the relevance of the new test to the case observed in the field. Thanks. ```