patch, 2 partsmidx: prevent against racily disappearing packs

Here are a couple of patches that Peff and I wrote after noticing a problem where racily disappearing .idx files can cause a segfault. While investigating, we fixed a related issue which is described and fixed in the second patch.

(I'm a little behind in responding to review on the reachability generation improvements patches, but had these in my backlog and they seemed relatively easy to send and remove from my TODO list. So, here they are :-).)

Thanks, Taylor

Taylor Blau (2):
  packfile.c: protect against disappearing indexes
  midx.c: protect against disappearing packs

 midx.c                      |  2 +-
 packfile.c                  | 19 ++-----------------
 t/t5319-multi-pack-index.sh | 30 ++++++++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 20 deletions(-)

-- 2.29.2.368.ge1806d1bdc

In 17c35c8969 (packfile: skip loading index if in multi-pack-index, 2018-07-12) we stopped loading the .idx file for packs that are contained within a multi-pack index.

This saves us the effort of loading an .idx and doing some lightweight validity checks by way of 'packfile.c:load_idx()', but introduces a race between processes that need to load the index (e.g., to generate a reverse index) and processes that can delete the index.

For example, running the following in your shell:

    $ git init repo && cd repo
    $ git commit --allow-empty -m 'base'
    $ git repack -ad && git multi-pack-index write

followed by:

    $ rm -f .git/objects/pack/pack-*.idx
    $ git rev-parse HEAD | git cat-file --batch-check='%(objectsize:disk)'

will result in a segfault prior to this patch. What's happening here is that we notice that the pack is in the multi-pack index, and so don't check that it still has a .idx. When we then try and load that index to generate a reverse index, we don't have it, so the call to 'find_pack_revindex()' in 'packfile.c:packed_object_info()' returns NULL, and then dereferencing it causes a segfault.

Of course, we don't ever expect someone to remove the index file by hand, or to be in a state where we never wrote it to begin with (yet find that pack in the multi-pack-index). But, this can happen in a timing race with 'git repack -ad', which removes all existing packs after writing a new pack containing all of their objects.

Avoid this by reverting the hunk of 17c35c8969 which stops loading the index when the pack is contained in a MIDX. This makes the latter half of 17c35c8969 useless, since we'll always have a non-NULL 'p->index_data', in which case that if statement isn't guarding anything.

These two together effectively revert 17c35c8969, and avoid the race explained above.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 packfile.c                  | 19 ++-----------------
 t/t5319-multi-pack-index.sh | 24 ++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 19 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Ah, OK. And demonstration using %(objectsize:disk) makes perfect sense given the above explanation.

Makes sense.  Thanks.

When a packed object is stored in a multi-pack index, but that pack has racily gone away, the MIDX code simply calls die(), when it could be returning an error to the caller, which would in turn lead to re-scanning the pack directory.

A pack can racily disappear, for example, due to a simultaneous 'git repack -ad',

You can also reproduce this with two terminals, where one is running:

    git init
    while true; do
      git commit -q --allow-empty -m foo
      git repack -ad
      git multi-pack-index write
    done

(in effect, constantly writing new MIDXs), and the other is running:

    obj=$(git rev-parse HEAD)
    while true; do
      echo $obj | git cat-file --batch-check='%(objectsize:disk)' || break
    done

That will sometimes hit the error preparing packfile from multi-pack-index message, which this patch fixes.

Right now, that path to discovering a missing pack looks something like 'find_pack_entry()' calling 'fill_midx_entry()' and eventually making its way to call 'nth_midxed_pack_entry()'.

'nth_midxed_pack_entry()' already checks 'is_pack_valid()' and propagates an error if the pack is invalid. So, this works if the pack has gone away between calling 'prepare_midx_pack()' and before calling 'is_pack_valid()', but not if it disappears before then.

Catch the case where the pack has already disappeared before 'prepare_midx_pack()' by returning an error in that case, too.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c                      | 2 +-
 t/t5319-multi-pack-index.sh | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Makes sense.  Will queue.

Thanks.

Taylor Blau <me@ttaylorr.com> writes:

In the cover letter it won't affect the end result, but when talking about "race", it always is a good idea to explicitly mention both sides of the race. It is clear what one side is in the above (i.e. somebody who removes .idx file that is still in use), but it is not so clear who gets hit and segfaults.

I am guessing that the other party is the user of .pack file(s) bypassing the corresponding .idx file(s) because the necessary data is mostly in .midx?

Thanks.

On Wed, Nov 25, 2020 at 01:13:31PM -0800, Junio C Hamano wrote:

Yeah, the race reproduction in the second commit message can actually reproduce the segfault as well (it depends on the exact timing which error you get). So the segfault is in the reader, who is not checking the result of find_revindex_entry().

Arguably every call there should be checking for NULL, but in practice I think it would always be a bug:

  - we were somehow unable to open the index in order to generate the
    revindex (which is what happened here). But I think we are better
    off making sure that we can always do so, which is what this series
    does.

  - the caller asked about an object at a position beyond the number of
    objects in the packfile. This is a bug in the caller.

So we could perhaps BUG() in find_revindex_entry() instead of returning NULL. A quick segfault accomplishes mostly the same thing, though the BUG() could distinguish the two cases more clearly.

-Peff

On Wed, Nov 25, 2020 at 07:48:54PM -0500, Jeff King wrote:

Yeah, a find_revindex_entry() that returns NULL means that the caller is probably dead in the water.

FWIW, this function gets touched by a series that I'm working on here: [1]. There, I think "returning NULL" is equivalent to "returning -1", and the problem exists there, too.

We could return a different negative number, call BUG(), or do nothing other than what's written. I don't have any strong feelings, though.

Thanks, Taylor

[1]: https://github.com/ttaylorr/git/blob/tb/on-disk-revindex-part-one/pack-revindex.c#L177-L201

On Wed, Nov 25, 2020 at 08:04:44PM -0500, Taylor Blau wrote:

Yeah, I was suggesting _never_ returning a failure, and just hitting a BUG() within the function. So it does not matter then how you represent the error return type, because there isn't one. :)

Looking over the callers, there are actually a few that check the return value and handle it sanely. I suspect they can never trigger in practice, given that the other callers would all segfault, and we have not seen any reports in the 15 years during which that has been the case. But perhaps some of them can be triggered by bogus pack data that nobody has ever run into in the real world.

At any rate, I am content to ignore it until somebody feels like digging into each caller. A BUG() is only marginally better than an immediate segfault anyway, and I'd prefer not to disrupt more substantive improvements in the area.

-Peff