Bugreport: pack-objects died of signal 11

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

On Tue, Jul 12, 2022 at 10:51:29AM +0200, Paul Horn wrote:

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 \
      </dev/null' \
    /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

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 <error: Cannot access 
memory at address 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=<optimized out>) at 
revision.c:2717
#9  setup_revisions (argc=61876512, argv=0x600003700000, revs=<optimized 
out>, revs@entry=0x1002a3c49, opt=<optimized out>, 
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=<optimized out>, 
argv=<optimized out>, prefix=<optimized out>) at builtin/pack-objects.c:4466
#12 0x0000000100005313 in run_builtin (p=0x600003b02920, 
p@entry=0xffffffff, argc=11, argc@entry=0, argv=<optimized out>, 
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 8/4/2022 2:21 PM, Emily Noneman wrote:

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

On Thu, Aug 04 2022, Derrick Stolee wrote:

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.

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 <commands+1992>,
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 <avarab@gmail.com> 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".

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 <git@knutwalker.engineer>
Reported-by: Emily Noneman <emily.noneman@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Thu, Aug 04 2022, Emily Noneman wrote:

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(-)

On 8/5/2022 10:24 AM, Ævar Arnfjörð Bjarmason wrote:

  
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

On Fri, Aug 05, 2022 at 10:48:43AM -0400, Derrick Stolee wrote:

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

Jeff King <peff@peff.net> writes:

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.

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.

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 <gitster@pobox.com> wrote:

On Mon, Aug 08, 2022 at 02:15:21PM -0400, Emily Noneman wrote:

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

Jeff King <peff@peff.net> writes:

Yes, it affects the relevance of the new test to the case observed in the field.

Thanks.

On Fri, Aug 05, 2022 at 09:41:57AM -0700, Junio C Hamano wrote:

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 <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7063-status-untracked-cache.sh | 5 +++++
 1 file changed, 5 insertions(+)

Jeff King <peff@peff.net> writes:

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.