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

[Jeff King <peff@peff.net>]

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