Re: [PATCH] revision.c: set-up "index_state.repo", don't segfault in pack-objects

From: Jeff King <peff@peff.net>
Date: Aug 5, 2022, 15:25 UTC
Message-ID: <Yu02dOo4G8sy8tI6@coredump.intra.peff.net>
In-Reply-To: <b6299f8a-f75a-0e96-a6a6-55a7280584bf@github.com>

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

Show 30 quoted lines

> 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