{"thread":{"id":"58140","subject":"Bugreport: pack-objects died of signal 11","startedAt":"2022-07-12T08:51:58Z","lastAt":"2022-08-09T16:56:16Z","messageCount":15,"participants":["Paul Horn","Jeff King","Emily Noneman","Derrick Stolee","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"458865","messageId":"977a6e0e-251c-4fa5-8b3b-fa3e2a761926@www.fastmail.com","threadId":"58140","inReplyTo":null,"subject":"Bugreport: pack-objects died of signal 11","fromName":"Paul Horn","fromEmail":"git@knutwalker.engineer","sentAt":"2022-07-12T08:51:29Z","receivedAt":"2022-07-12T08:51:58Z","isPatch":false,"sender":{"key":"git@knutwalker.engineer","avatar":null},"body":"Hi,\n\nultimately, `git repack` segfaults:\n\n $ git repack\n error: pack-objects died of signal 11\n\nThis also errors out GC runs and breaks index maintenance, though other commands still succeed.\n\n $ cat $(git rev-parse --git-dir)/gc.log\n error: pack-objects died of signal 11\n fatal: failed to run repack\n\n\nThe repository is a bare repository with 11 worktrees. I can only observe this error in this repository, others do not reproduce it.\nThe attached log contains the output of `GIT_TRACE=1 GIT_TRACE_PACK_ACCESS=1 git repack`\n\n\n[System Info]\ngit version:\ngit version 2.37.0\ncpu: arm64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: 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\ncompiler info: clang: 13.1.6 (clang-1316.0.21.2.5)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /opt/homebrew/bin/bash\n\n\n[Enabled Hooks]\n\n\n\nCheers,\nPaul"},{"id":"459093","messageId":"YtCMklbIoTAN/WRs@coredump.intra.peff.net","threadId":"58140","inReplyTo":"977a6e0e-251c-4fa5-8b3b-fa3e2a761926@www.fastmail.com","subject":"Re: Bugreport: pack-objects died of signal 11","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-14T21:37:22Z","receivedAt":"2022-07-14T21:37:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 12, 2022 at 10:51:29AM +0200, Paul Horn wrote:\n\n> ultimately, `git repack` segfaults:\n> \n> $ git repack\n> error: pack-objects died of signal 11\n\nCan you provide a backtrace that shows where we segfaulted? If you can\nbuild Git from source and have access to a debugger like gdb, that\nshould be enough.\n\nOne option is to make sure core dumps are enabled (probably \"ulimit -c\nunlimited\"), then run the failing repack, then find the core file\n(usually in the current directory, but I'm not sure offhand about\nmacOS), and then open the debugger on it (\"gdb /path/to/your/git\n/path/to/core\").\n\nBut perhaps easier may be to directly run the buggy pack-objects\ninvocation. I pulled the options here from your trace file. Something\nlike:\n\n gdb -ex 'set args pack-objects \\\n --delta-base-offset tmp-pack \\\n --keep-true-parents --honor-pack-keep --non-empty \\\n --all --reflog --indexed-objects --unpacked --incremental \\\n config && repo->config->hash_initialized)\n(gdb) bt\n#0  git_config_check_init (repo=0x0) at config.c:2538\n#1  0x00000001001197a8 in repo_config_get_string (repo=0x0, \nkey=0x1002a3c49 \"status.showuntrackedfiles\", dest=0x0, \ndest@entry=0x7ff7bfefc1f0) at config.c:2574\n#2  0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at \ndir.c:2781\n#3  new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797\n#4  0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) \nat read-cache.c:1996\n#5  post_read_index_from (istate=0x7ff7bfefc7e0, \nistate@entry=0x600003b02920) at read-cache.c:2028\n#6  0x00000001001d6599 in read_index_from (istate=0x600003b02920, \nistate@entry=0x0, path=path@entry=0xffffffff ,\n     gitdir=0x600002c08080 \n\"/Users/emily.noneman/workspace/webdev/.NetLedger_LocalBranchData.git\") \nat read-cache.c:2457\n#7  0x00000001001fcb2d in add_index_objects_to_pending \n(revs=0x600003b02920, revs@entry=0x7ff7bfefe9a0, flags=flags@entry=0) at \nrevision.c:1783\n#8  0x00000001002001e4 in handle_revision_pseudo_opt \n(revs=0x7ff7bfefe9a0, argv=0x600003700020, flags=) at \nrevision.c:2717\n#9  setup_revisions (argc=61876512, argv=0x600003700000, revs=, revs@entry=0x1002a3c49, opt=, \nopt@entry=0x7ff7bfefca50) at revision.c:2806\n#10 0x0000000100084092 in get_object_list (revs=0x1002a3c49, \nrevs@entry=0x0, ac=0, av=0x1002a3c49) at builtin/pack-objects.c:3993\n#11 0x0000000100080a07 in cmd_pack_objects (argc=, \nargv=, prefix=) at builtin/pack-objects.c:4466\n#12 0x0000000100005313 in run_builtin (p=0x600003b02920, \np@entry=0xffffffff, argc=11, argc@entry=0, argv=, \nargv@entry=0x1002a3c49) at git.c:466\n#13 0x0000000100004566 in handle_builtin (argc=0, argv=0x1002a3c49) at \ngit.c:720\n#14 0x0000000100003b59 in run_argv (argcp=0x7ff7bfeff7a4, \nargv=0x7ff7bfeff798) at git.c:787\n#15 cmd_main (argc=11, argc@entry=0, argv=0x7ff7bfeff968, \nargv@entry=0x1002a3c49) at git.c:920\n#16 0x00000001000d49a5 in main (argc=0, argv=0x1002a3c49) at \ncommon-main.c:56\n\nOn 7/14/22 5:37 PM, Jeff King wrote:\n> On Tue, Jul 12, 2022 at 10:51:29AM +0200, Paul Horn wrote:\n>\n>> ultimately, `git repack` segfaults:\n>>\n>> $ git repack\n>> error: pack-objects died of signal 11\n> Can you provide a backtrace that shows where we segfaulted? If you can\n> build Git from source and have access to a debugger like gdb, that\n> should be enough.\n>\n> One option is to make sure core dumps are enabled (probably \"ulimit -c\n> unlimited\"), then run the failing repack, then find the core file\n> (usually in the current directory, but I'm not sure offhand about\n> macOS), and then open the debugger on it (\"gdb /path/to/your/git\n> /path/to/core\").\n>\n> But perhaps easier may be to directly run the buggy pack-objects\n> invocation. I pulled the options here from your trace file. Something\n> like:\n>\n> gdb -ex 'set args pack-objects \\\n> --delta-base-offset tmp-pack \\\n> --keep-true-parents --honor-pack-keep --non-empty \\\n> --all --reflog --indexed-objects --unpacked --incremental \\\n> /path/to/your/git\n>\n> Then \"r\" to run inside the debugger, which will stop if we segfault. And\n> then \"bt\" will show the backtrace.\n>\n> Alternatively, is it possible to make the offending repository available\n> (even off-list)?\n>\n> -Peff\n>\n>\n"},{"id":"460658","messageId":"87d46db2-8e09-e5a7-b6bb-e94bf05df305@github.com","threadId":"58140","inReplyTo":"ecb6fa27-b918-4234-8e44-13c2a3e76e07@gmail.com","subject":"Re: Bugreport: pack-objects died of signal 11","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-04T20:14:03Z","receivedAt":"2022-08-04T20:14:08Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/4/2022 2:21 PM, Emily Noneman wrote:\n> The stack trace:\n> Thread 2 received signal SIGSEGV, Segmentation fault.\n> git_config_check_init (repo=0x0) at config.c:2538\n> 2538        if (repo->config && repo->config->hash_initialized)\n> (gdb) bt\n> #0  git_config_check_init (repo=0x0) at config.c:2538\n> #1  0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 \"status.showuntrackedfiles\", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574\n> #2  0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781\n> #3  new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797\n> #4  0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996\n\nHere is where things are confusing:\n\n* tweak_untracked_cache() takes an 'istate' that is non-NULL here.\n\n* The next spot in the stack is new_untracked_cache() with a NULL 'istate'.\n\nThe only way these are connected is by a missing stack frame (probably\noptimized out) calling add_untracked_cache(). Still, it should be\npassing 'istate' throughout this process.\n\nThe repo_config_get_string() call must also be coming from\nnew_untracked_cache_flags() which is again a missing stack frame,\nbut is called from new_untracked_cache(). Strangely, it's using\na NULL 'repo' here which should have come from 'istate->repo', so\nwe should have had a segfault earlier.\n\nSorry for the drive-by commentary without any solution. This is\njust genuinely puzzling to me.\n\nThanks,\n-Stolee\n\n"},{"id":"460672","messageId":"220804.86tu6rso6n.gmgdl@evledraar.gmail.com","threadId":"58140","inReplyTo":"87d46db2-8e09-e5a7-b6bb-e94bf05df305@github.com","subject":"Re: Bugreport: pack-objects died of signal 11","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-04T21:07:16Z","receivedAt":"2022-08-04T21:18:15Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 04 2022, Derrick Stolee wrote:\n\n> On 8/4/2022 2:21 PM, Emily Noneman wrote:\n>> The stack trace:\n>> Thread 2 received signal SIGSEGV, Segmentation fault.\n>> git_config_check_init (repo=0x0) at config.c:2538\n>> 2538        if (repo->config && repo->config->hash_initialized)\n>> (gdb) bt\n>> #0  git_config_check_init (repo=0x0) at config.c:2538\n>> #1  0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 \"status.showuntrackedfiles\", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574\n>> #2  0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781\n>> #3  new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797\n>> #4  0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996\n>\n> Here is where things are confusing:\n>\n> * tweak_untracked_cache() takes an 'istate' that is non-NULL here.\n>\n> * The next spot in the stack is new_untracked_cache() with a NULL 'istate'.\n>\n> The only way these are connected is by a missing stack frame (probably\n> optimized out) calling add_untracked_cache(). Still, it should be\n> passing 'istate' throughout this process.\n>\n> The repo_config_get_string() call must also be coming from\n> new_untracked_cache_flags() which is again a missing stack frame,\n> but is called from new_untracked_cache(). Strangely, it's using\n> a NULL 'repo' here which should have come from 'istate->repo', so\n> we should have had a segfault earlier.\n>\n> Sorry for the drive-by commentary without any solution. This is\n> just genuinely puzzling to me.\n\nI think this segfault might be fixed by this patch of mine, which I\nwrote for something unrelated back in April (but it was never sent to\nthe list).\n\n\thttps://github.com/avar/git/commit/d83bfa866ba\n\nEmily and/or Paul: Are you able to test the patch to see if it would\nwork, diff here: https://github.com/avar/git/commit/d83bfa866ba.patch\n\nIt's exactly on the codepath in this stacktrace,\ni.e. add_index_objects_to_pending() in revision.c will do before/after:\n\n\t- struct index_state istate = { NULL };\n\t+ struct index_state istate = { .repo = revs->repo };\n\nThen when we're all the way down in new_untracked_cache_flags() we do:\n\n\tstruct repository *repo = istate->repo;\n\nWhich then calls (indirectly) git_config_check_init(), and we segfault\nnot because istate is NULL, but because the \"repo\" it's carrying is\nNULL.\n\nBut maybe I'm wrong, I haven't been able to reproduce this.\n\nThe reason I wrote that patch (as can be seen if you peek at the WIP\nbranch it's at) is because I ran into a similar dependency between\nthe_index and the_repo & an istate variable being passed around with\nfsmonitor-settings.c.\n"},{"id":"460680","messageId":"CAKvVO18RVye=PkBRv=trj2GHh8ccGKL5j0mMq2eHQ1SX=wsr8A@mail.gmail.com","threadId":"58140","inReplyTo":"220804.86tu6rso6n.gmgdl@evledraar.gmail.com","subject":"Re: Bugreport: pack-objects died of signal 11","fromName":"Emily Noneman","fromEmail":"emily.noneman@gmail.com","sentAt":"2022-08-04T22:32:44Z","receivedAt":"2022-08-04T22:33:00Z","isPatch":false,"sender":{"key":"emily.noneman@gmail.com","avatar":"https://gravatar.com/avatar/a00071035a9ecfbfa1eebde61d0e333be38de30b3251d547f1f4fdf35a124c23?d=mp&s=160"},"body":"I have an updated backtrace with optimizations turned off:\nThread 2 received signal SIGSEGV, Segmentation fault.\n0x000000010016f770 in git_config_check_init (repo=0x0) at config.c:2538\n2538 if (repo->config && repo->config->hash_initialized)\n(gdb) bt\n#0 0x000000010016f770 in git_config_check_init (repo=0x0) at config.c:2538\n#1 0x000000010016f94d in repo_config_get_string (repo=0x0,\nkey=0x10036e541 \"status.showuntrackedfiles\", dest=0x7ff7bfefc6d0) at\nconfig.c:2574\n#2 0x00000001001b4dbf in new_untracked_cache_flags\n(istate=0x7ff7bfefc858) at dir.c:2781\n#3 0x00000001001b0027 in new_untracked_cache (istate=0x7ff7bfefc858,\nflags=-1) at dir.c:2797\n#4 0x00000001001aff6c in add_untracked_cache (istate=0x7ff7bfefc858)\nat dir.c:2806\n#5 0x0000000100276126 in tweak_untracked_cache\n(istate=0x7ff7bfefc858) at read-cache.c:1996\n#6 0x000000010027352e in post_read_index_from (istate=0x7ff7bfefc858)\nat read-cache.c:2028\n#7 0x0000000100273228 in read_index_from (istate=0x7ff7bfefc858,\npath=0x600002c0ac00\n\"/Users/emily.noneman/workspace/webdev/.NetLedger_LocalBranchData.git/index\",\n gitdir=0x600002c04080\n\"/Users/emily.noneman/workspace/webdev/.NetLedger_LocalBranchData.git\")\nat read-cache.c:2457\n#8 0x00000001002a98c1 in add_index_objects_to_pending\n(revs=0x7ff7bfefcfe8, flags=0) at revision.c:1783\n#9 0x00000001002adb4b in handle_revision_pseudo_opt\n(revs=0x7ff7bfefcfe8, argv=0x600003704020, flags=0x7ff7bfefcaf8) at\nrevision.c:2717\n#10 0x00000001002acd2c in setup_revisions (argc=6,\nargv=0x600003704000, revs=0x7ff7bfefcfe8, opt=0x7ff7bfefcb78) at\nrevision.c:2806\n#11 0x00000001000a747e in get_object_list (revs=0x7ff7bfefcfe8, ac=6,\nav=0x600003704000) at builtin/pack-objects.c:3993\n#12 0x00000001000a5f30 in cmd_pack_objects (argc=0,\nargv=0x7ff7bfeff968, prefix=0x0) at builtin/pack-objects.c:4472\n#13 0x0000000100002a03 in run_builtin (p=0x1003d0fb8 ,\nargc=11, argv=0x7ff7bfeff968) at git.c:466\n#14 0x0000000100001528 in handle_builtin (argc=11,\nargv=0x7ff7bfeff968) at git.c:720\n#15 0x0000000100002406 in run_argv (argcp=0x7ff7bfeff7dc,\nargv=0x7ff7bfeff7d0) at git.c:787\n#16 0x00000001000012f9 in cmd_main (argc=11, argv=0x7ff7bfeff968) at git.c:920\n#17 0x00000001001155c6 in main (argc=12, argv=0x7ff7bfeff960) at\ncommon-main.c:56\n\nÆvar Arnfjörð Bjarmason, your patch fixed the issue for me! Thank you!\n\n\nOn Thu, Aug 4, 2022 at 5:18 PM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Thu, Aug 04 2022, Derrick Stolee wrote:\n>\n> > On 8/4/2022 2:21 PM, Emily Noneman wrote:\n> >> The stack trace:\n> >> Thread 2 received signal SIGSEGV, Segmentation fault.\n> >> git_config_check_init (repo=0x0) at config.c:2538\n> >> 2538 if (repo->config && repo->config->hash_initialized)\n> >> (gdb) bt\n> >> #0 git_config_check_init (repo=0x0) at config.c:2538\n> >> #1 0x00000001001197a8 in repo_config_get_string (repo=0x0, key=0x1002a3c49 \"status.showuntrackedfiles\", dest=0x0, dest@entry=0x7ff7bfefc1f0) at config.c:2574\n> >> #2 0x000000010014a85b in new_untracked_cache_flags (istate=0x0) at dir.c:2781\n> >> #3 new_untracked_cache (istate=0x0, flags=-1) at dir.c:2797\n> >> #4 0x00000001001d68f1 in tweak_untracked_cache (istate=0x7ff7bfefc7e0) at read-cache.c:1996\n> >\n> > Here is where things are confusing:\n> >\n> > * tweak_untracked_cache() takes an 'istate' that is non-NULL here.\n> >\n> > * The next spot in the stack is new_untracked_cache() with a NULL 'istate'.\n> >\n> > The only way these are connected is by a missing stack frame (probably\n> > optimized out) calling add_untracked_cache(). Still, it should be\n> > passing 'istate' throughout this process.\n> >\n> > The repo_config_get_string() call must also be coming from\n> > new_untracked_cache_flags() which is again a missing stack frame,\n> > but is called from new_untracked_cache(). Strangely, it's using\n> > a NULL 'repo' here which should have come from 'istate->repo', so\n> > we should have had a segfault earlier.\n> >\n> > Sorry for the drive-by commentary without any solution. This is\n> > just genuinely puzzling to me.\n>\n> I think this segfault might be fixed by this patch of mine, which I\n> wrote for something unrelated back in April (but it was never sent to\n> the list).\n>\n> https://github.com/avar/git/commit/d83bfa866ba\n>\n> Emily and/or Paul: Are you able to test the patch to see if it would\n> work, diff here: https://github.com/avar/git/commit/d83bfa866ba.patch\n>\n> It's exactly on the codepath in this stacktrace,\n> i.e. add_index_objects_to_pending() in revision.c will do before/after:\n>\n> - struct index_state istate = { NULL };\n> + struct index_state istate = { .repo = revs->repo };\n>\n> Then when we're all the way down in new_untracked_cache_flags() we do:\n>\n> struct repository *repo = istate->repo;\n>\n> Which then calls (indirectly) git_config_check_init(), and we segfault\n> not because istate is NULL, but because the \"repo\" it's carrying is\n> NULL.\n>\n> But maybe I'm wrong, I haven't been able to reproduce this.\n>\n> The reason I wrote that patch (as can be seen if you peek at the WIP\n> branch it's at) is because I ran into a similar dependency between\n> the_index and the_repo & an istate variable being passed around with\n> fsmonitor-settings.c.\n"},{"id":"460709","messageId":"patch-1.1-3ff17707481-20220805T141816Z-avarab@gmail.com","threadId":"58140","inReplyTo":"CAKvVO18RVye=PkBRv=trj2GHh8ccGKL5j0mMq2eHQ1SX=wsr8A@mail.gmail.com","subject":"[PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-05T14:24:34Z","receivedAt":"2022-08-05T14:25:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Narrowly fix a segfault that's been reported against pack-objects at\n[1] and [2].\n\nThis fixes a segfault in \"git pack-objects\", see a reports at [1] and\n[2]. In that case we'd go through the add_index_objects_to_pending()\ncodepath being modified here, and eventually call into\nnew_untracked_cache().\n\nWe'd then segfault in new_untracked_cache_flags() as we tried to use a\nNULL \"repo\" extracted from the \"struct index_state\".\n\nThe segfault we ran into there is only a symptom of a much bigger\nproblem, namely that we have various \"struct index_state\" variables\nconstructed in various places, and then call into functions that might\nexpect to have a non-NULL \"repo\" member. In follow-up work we'll fix\nthat more general problem, but let's just fix the segfault for now.\n\n1. https://lore.kernel.org/git/977a6e0e-251c-4fa5-8b3b-fa3e2a761926@www.fastmail.com/\n2. https://lore.kernel.org/git/ecb6fa27-b918-4234-8e44-13c2a3e76e07@gmail.com/\n\nReported-by: Paul Horn \nReported-by: Emily Noneman \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Thu, Aug 04 2022, Emily Noneman wrote:\n> Ævar Arnfjörð Bjarmason, your patch fixed the issue for me! Thank you!\n\nThanks, would you mind testing this more narrow fix, I think it should\nfix the segfault too, but let's be sure & confirm it.\n\nI haven't (and haven't really tried) to come up with a regression fix\nfor this. I have changes that come after this which extensively test\nthe same issue elsewhere, so maybe it's not needed...\n\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 87f1c117d13..48c2f1b91a8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1775,7 +1775,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n \tworktrees = get_worktrees();\n \tfor (p = worktrees; *p; p++) {\n \t\tstruct worktree *wt = *p;\n-\t\tstruct index_state istate = { NULL };\n+\t\tstruct index_state istate = { .repo = revs->repo };\n \n \t\tif (wt->is_current)\n \t\t\tcontinue; /* current index already taken care of */\n-- \n2.37.1.1283.g2c56c4202c5\n\n"},{"id":"460711","messageId":"b6299f8a-f75a-0e96-a6a6-55a7280584bf@github.com","threadId":"58140","inReplyTo":"patch-1.1-3ff17707481-20220805T141816Z-avarab@gmail.com","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-05T14:48:43Z","receivedAt":"2022-08-05T14:48:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/5/2022 10:24 AM, Ævar Arnfjörð Bjarmason wrote:\n> Narrowly fix a segfault that's been reported against pack-objects at\n> [1] and [2].\n> \n> This fixes a segfault in \"git pack-objects\", see a reports at [1] and\n> [2]. In that case we'd go through the add_index_objects_to_pending()\n> codepath being modified here, and eventually call into\n> new_untracked_cache().\n> \n> We'd then segfault in new_untracked_cache_flags() as we tried to use a\n> NULL \"repo\" extracted from the \"struct index_state\".\n\n> \t\tstruct worktree *wt = *p;\n> -\t\tstruct index_state istate = { NULL };\n> +\t\tstruct index_state istate = { .repo = revs->repo };\n \nThanks for the fix. It is definitely narrow to this particular\nstack trace.\n\nIt is rare that we create an index that is not 'the_index' in\nthis way. However, there are similar { NULL } initializers that\ncould be cleaned up in these files:\n\n * apply.c\n * merge-recursive.c\n * builtin/stash.c\n * builtin/worktree.c\n\nOf course, delay these for a follow-up since these uses are\nnot causing segfaults in the wild.\n\nThanks,\n-Stolee\n\n"},{"id":"460714","messageId":"Yu02dOo4G8sy8tI6@coredump.intra.peff.net","threadId":"58140","inReplyTo":"b6299f8a-f75a-0e96-a6a6-55a7280584bf@github.com","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-05T15:25:40Z","receivedAt":"2022-08-05T15:25:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 05, 2022 at 10:48:43AM -0400, Derrick Stolee wrote:\n\n> On 8/5/2022 10:24 AM, Ævar Arnfjörð Bjarmason wrote:\n> > Narrowly fix a segfault that's been reported against pack-objects at\n> > [1] and [2].\n> > \n> > This fixes a segfault in \"git pack-objects\", see a reports at [1] and\n> > [2]. In that case we'd go through the add_index_objects_to_pending()\n> > codepath being modified here, and eventually call into\n> > new_untracked_cache().\n> > \n> > We'd then segfault in new_untracked_cache_flags() as we tried to use a\n> > NULL \"repo\" extracted from the \"struct index_state\".\n> \n> > \t\tstruct worktree *wt = *p;\n> > -\t\tstruct index_state istate = { NULL };\n> > +\t\tstruct index_state istate = { .repo = revs->repo };\n> \n> Thanks for the fix. It is definitely narrow to this particular\n> stack trace.\n> \n> It is rare that we create an index that is not 'the_index' in\n> this way. However, there are similar { NULL } initializers that\n> could be cleaned up in these files:\n> \n> * apply.c\n> * merge-recursive.c\n> * builtin/stash.c\n> * builtin/worktree.c\n> \n> Of course, delay these for a follow-up since these uses are\n> not causing segfaults in the wild.\n\nWe did have another similar segfault recently:\n\n https://lore.kernel.org/git/YtrdPguYs3a3xekv@kitenet.net/\n\nIt's not quite the same case, but I think there's some overlap, and the\nreason Ævar couldn't reproduce is that Martin's patch in that thread has\nalready fixed this case, too.\n\nUsing git v2.37.1, I can get a segfault with a backtrace similar to\nEmily's by running:\n\n git init repo\n cd repo\n git commit --allow-empty -m base\n\n git config core.untrackedCache true\n git worktree add at\n rm .git/worktrees/wt/index\n\n git gc\n\nAnd applying the patch from this thread does make it go away. But\nlikewise for cherry-picking 4447d4129d (read-cache: make `do_read_index()`\nalways set up `istate->repo`, 2022-07-22), which is already in \"master\".\n\nOf the two patches, I think 4447d4129d is the better approach. The\nassumption in the code seems to be that do_read_index() (and thus\nread_index_from(), etc) will set up istate->repo. That patch fixes a\ncorner case where we failed to do so. And with that fix, there's no need\nfor the callers to set things up ahead of time. So it covers all of\nthose initializers you mentioned.\n\nEmily, Paul: I'm 99% sure this will be the case given my reproduction\nabove, but if you could try reproducing the problem with the current tip\nof \"master\" from git.git, that would confirm the findings.\n\n-Peff\n"},{"id":"460721","messageId":"xmqqo7wyzlpm.fsf@gitster.g","threadId":"58140","inReplyTo":"Yu02dOo4G8sy8tI6@coredump.intra.peff.net","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-05T16:41:57Z","receivedAt":"2022-08-05T16:42:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Of the two patches, I think 4447d4129d is the better approach. The\n> assumption in the code seems to be that do_read_index() (and thus\n> read_index_from(), etc) will set up istate->repo. That patch fixes a\n> corner case where we failed to do so. And with that fix, there's no need\n> for the callers to set things up ahead of time. So it covers all of\n> those initializers you mentioned.\n\nYeah, I tend to agree that Martin's fix, which is a more focused\none, is the better approach between the two. It was merged to\nthe 'master' track only a few days ago.\n\nWe are at the end of the week #4 of this 12-week cycle, and we've\naccumulated about two dozens of topics already on 'maint' since the\nfirst maintenance release v2.37.1 was done, so we may have a chance\nto merge Martin's fix down to 'maint' before tagging the v2.37.2\nrelease.\n\nOne thing that may help is to add a test similar to the situation\nEmily & Paul had to t7063 on top of 4447d4129d before merging it\ndown, perhaps?\n\nIn your reproduction, the \"rm .git/index\" step makes the worktree's\nbranch \"not checked out\" (the commit is empty so \"worktree add\" may\ncheck out no files, and removing the index will make it as if you\ndid \"clone --no-checkout\") ...\n\n git init repo\n cd repo\n git commit --allow-empty -m base\n\n git config core.untrackedCache true\n git worktree add at\n rm .git/worktrees/wt/index\n\n git gc\n\n... but it is not something an end user is likely to do, so I am\nstill curious how this was triggered in real life.\n\nAh, OK, I think the steps can be tweaked to\n\n git config core.untrackedCache true\n - git worktree add at\n - rm .git/worktrees/wt/index\n + git worktree add --no-checkout wt\n\n git gc\n\ni.e. (1) With \"worktree add --no-checkout\", there is no need to\nmanually remove the index file, and (2) \"at\" is an obvious typo of\n\"wt\". This does not require the history to be a singleton empty\ntree, either.\n\nAnd that is a less implausible thing for users to be doing, but may\nstill not be very common. Perhaps the --no-checkout is a prelude to\nset up a custom sparse checkout pattern, to avoid a wasteful full\ntree checkout before doing so, or something? If that is the case,\nthen the above sequence becomes a very plausible thing for users to\nbe doing.\n\n> Emily, Paul: I'm 99% sure this will be the case given my reproduction\n> above, but if you could try reproducing the problem with the current tip\n> of \"master\" from git.git, that would confirm the findings.\n\nYes, indded. That would be very helpful.\n\nThanks, all, for discussing the problem and its solution(s), and\nthanks in advance for further testing to help the fix forward.\n"},{"id":"460843","messageId":"CAKvVO18Du5M6broAJ6WqTAk6C4jxtqbXoS=9x03JgSdJAak80Q@mail.gmail.com","threadId":"58140","inReplyTo":"xmqqo7wyzlpm.fsf@gitster.g","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Emily Noneman","fromEmail":"emily.noneman@gmail.com","sentAt":"2022-08-08T18:15:21Z","receivedAt":"2022-08-08T18:15:56Z","isPatch":true,"sender":{"key":"emily.noneman@gmail.com","avatar":"https://gravatar.com/avatar/a00071035a9ecfbfa1eebde61d0e333be38de30b3251d547f1f4fdf35a124c23?d=mp&s=160"},"body":"The latest changes seem to have resolved the issue for me. My repo was\na bit different than Pauls, it's a big corporate repo with a partial\ncheckout and a worktree. What's confusing is that I do see an \"index\"\nfile in worktrees/NetLedger_GitRepo , but git 2.37.1 exhibits the\ncrash. 2.37.1.377.g679aad9e82 works just fine.\n\nOn Fri, Aug 5, 2022 at 12:42 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > Of the two patches, I think 4447d4129d is the better approach. The\n> > assumption in the code seems to be that do_read_index() (and thus\n> > read_index_from(), etc) will set up istate->repo. That patch fixes a\n> > corner case where we failed to do so. And with that fix, there's no need\n> > for the callers to set things up ahead of time. So it covers all of\n> > those initializers you mentioned.\n>\n> Yeah, I tend to agree that Martin's fix, which is a more focused\n> one, is the better approach between the two. It was merged to\n> the 'master' track only a few days ago.\n>\n> We are at the end of the week #4 of this 12-week cycle, and we've\n> accumulated about two dozens of topics already on 'maint' since the\n> first maintenance release v2.37.1 was done, so we may have a chance\n> to merge Martin's fix down to 'maint' before tagging the v2.37.2\n> release.\n>\n> One thing that may help is to add a test similar to the situation\n> Emily & Paul had to t7063 on top of 4447d4129d before merging it\n> down, perhaps?\n>\n> In your reproduction, the \"rm .git/index\" step makes the worktree's\n> branch \"not checked out\" (the commit is empty so \"worktree add\" may\n> check out no files, and removing the index will make it as if you\n> did \"clone --no-checkout\") ...\n>\n> git init repo\n> cd repo\n> git commit --allow-empty -m base\n>\n> git config core.untrackedCache true\n> git worktree add at\n> rm .git/worktrees/wt/index\n>\n> git gc\n>\n> ... but it is not something an end user is likely to do, so I am\n> still curious how this was triggered in real life.\n>\n> Ah, OK, I think the steps can be tweaked to\n>\n> git config core.untrackedCache true\n> - git worktree add at\n> - rm .git/worktrees/wt/index\n> + git worktree add --no-checkout wt\n>\n> git gc\n>\n> i.e. (1) With \"worktree add --no-checkout\", there is no need to\n> manually remove the index file, and (2) \"at\" is an obvious typo of\n> \"wt\". This does not require the history to be a singleton empty\n> tree, either.\n>\n> And that is a less implausible thing for users to be doing, but may\n> still not be very common. Perhaps the --no-checkout is a prelude to\n> set up a custom sparse checkout pattern, to avoid a wasteful full\n> tree checkout before doing so, or something? If that is the case,\n> then the above sequence becomes a very plausible thing for users to\n> be doing.\n>\n> > Emily, Paul: I'm 99% sure this will be the case given my reproduction\n> > above, but if you could try reproducing the problem with the current tip\n> > of \"master\" from git.git, that would confirm the findings.\n>\n> Yes, indded. That would be very helpful.\n>\n> Thanks, all, for discussing the problem and its solution(s), and\n> thanks in advance for further testing to help the fix forward.\n"},{"id":"460902","messageId":"YvJZCGyhd6KJCabi@coredump.intra.peff.net","threadId":"58140","inReplyTo":"xmqqo7wyzlpm.fsf@gitster.g","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-09T12:54:32Z","receivedAt":"2022-08-09T12:54:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 05, 2022 at 09:41:57AM -0700, Junio C Hamano wrote:\n\n> Ah, OK, I think the steps can be tweaked to\n> \n> git config core.untrackedCache true\n> - git worktree add at\n> - rm .git/worktrees/wt/index\n> + git worktree add --no-checkout wt\n> \n> git gc\n> \n> i.e. (1) With \"worktree add --no-checkout\", there is no need to\n> manually remove the index file, and (2) \"at\" is an obvious typo of\n> \"wt\". This does not require the history to be a singleton empty\n> tree, either.\n\nThat makes sense. I worked backwards from the guess that Martin's patch\nwas relevant, hence trying to trigger ENOENT on the file. Using\n--no-checkout is a plausible way to get there. (And yes, the \"at\" is a\ntypo. I double-checked the script I actually ran and it did not have the\ntypo; not sure how I managed to introduce it via cut and paste!).\n\nThe singleton empty tree was only there in my example because \"git\nworktree\" insists on having a starting point. Its contents shouldn't\nmatter either way.\n\nHere's the test addition you mentioned, though I'm really not sure it's\nadding much. The source lines for both the bug/fix and the spot where we\nsegfault should be identical between the two cases. It's really just\nabout how we ended up calling into the code.\n\n-- >8 --\nSubject: [PATCH] t7063: add another missing-index test\n\nCommit 4447d4129d (read-cache: make `do_read_index()` always set up\n`istate->repo`, 2022-07-22) fixed a case where a missing \"index\" file\ncaused us to segfault in the untracked-cache code.\n\nWe ran into another situation in the wild which caused a similar\nsegfault, this time with an empty worktree. These actually segfault at\nthe same spot (inside the index reading code), but the call path to get\nthere is different between the two, hence it may be worth checking both.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Jeff King \n---\n t/t7063-status-untracked-cache.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex c1f0d95036..a0e8920ffa 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -990,4 +990,9 @@ test_expect_success 'empty repo (no index) and core.untrackedCache' '\n \tgit -C emptyrepo -c core.untrackedCache=true write-tree\n '\n \n+test_expect_success 'empty worktree and core.untrackedCache' '\n+\tgit worktree add --no-checkout empty-worktree &&\n+\tgit -c core.untrackedCache=true gc\n+'\n+\n test_done\n-- \n2.37.1.845.ge5d2b690ea\n\n"},{"id":"460903","messageId":"YvJZWUVULB5Y8zih@coredump.intra.peff.net","threadId":"58140","inReplyTo":"CAKvVO18Du5M6broAJ6WqTAk6C4jxtqbXoS=9x03JgSdJAak80Q@mail.gmail.com","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-09T12:55:53Z","receivedAt":"2022-08-09T12:56:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2022 at 02:15:21PM -0400, Emily Noneman wrote:\n\n> The latest changes seem to have resolved the issue for me. My repo was\n> a bit different than Pauls, it's a big corporate repo with a partial\n> checkout and a worktree. What's confusing is that I do see an \"index\"\n> file in worktrees/NetLedger_GitRepo , but git 2.37.1 exhibits the\n> crash. 2.37.1.377.g679aad9e82 works just fine.\n\nThanks for testing. It is odd that you see an \"index\" file, since the\ncode change is definitely focused around getting ENOENT from open() on\nthe index. Is it possible that you have another worktree (even the\ninitial one) without an index in it?\n\n-Peff\n"},{"id":"460926","messageId":"xmqqedxpqrvc.fsf@gitster.g","threadId":"58140","inReplyTo":"YvJZCGyhd6KJCabi@coredump.intra.peff.net","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-09T16:55:03Z","receivedAt":"2022-08-09T16:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's the test addition you mentioned, though I'm really not sure it's\n> adding much. The source lines for both the bug/fix and the spot where we\n> segfault should be identical between the two cases. It's really just\n> about how we ended up calling into the code.\n\nTrue, but the \"non-existing index file\" case is rare enough that it\nwould be worth to document the new way to get into the state, I\nwould think.\n\nI am offline every other Tuesday and today is such a day,\nso please expect no changes to the public repositories.\n\nThanks.\n\n> t/t7063-status-untracked-cache.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n> index c1f0d95036..a0e8920ffa 100755\n> --- a/t/t7063-status-untracked-cache.sh\n> +++ b/t/t7063-status-untracked-cache.sh\n> @@ -990,4 +990,9 @@ test_expect_success 'empty repo (no index) and core.untrackedCache' '\n> \tgit -C emptyrepo -c core.untrackedCache=true write-tree\n> '\n> \n> +test_expect_success 'empty worktree and core.untrackedCache' '\n> +\tgit worktree add --no-checkout empty-worktree &&\n> +\tgit -c core.untrackedCache=true gc\n> +'\n> +\n> test_done\n"},{"id":"460927","messageId":"xmqqa68dqrtj.fsf@gitster.g","threadId":"58140","inReplyTo":"YvJZWUVULB5Y8zih@coredump.intra.peff.net","subject":"Re: [PATCH] revision.c: set-up \"index_state.repo\", don't segfault in pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-09T16:56:08Z","receivedAt":"2022-08-09T16:56:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 08, 2022 at 02:15:21PM -0400, Emily Noneman wrote:\n>\n>> The latest changes seem to have resolved the issue for me. My repo was\n>> a bit different than Pauls, it's a big corporate repo with a partial\n>> checkout and a worktree. What's confusing is that I do see an \"index\"\n>> file in worktrees/NetLedger_GitRepo , but git 2.37.1 exhibits the\n>> crash. 2.37.1.377.g679aad9e82 works just fine.\n>\n> Thanks for testing. It is odd that you see an \"index\" file, since the\n> code change is definitely focused around getting ENOENT from open() on\n> the index. Is it possible that you have another worktree (even the\n> initial one) without an index in it?\n\nYes, it affects the relevance of the new test to the case observed\nin the field.\n\nThanks.\n"}]}