packfile.c: protect against disappearing indexes

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Nov 25, 2020 at 12:17 UTC c8a45eb66e0a1086a31af4d76cd0f5e228497229
2 files changed +24 -19
packfile.c
+2 -17
@@ -514,19 +514,8 @@ static int open_packed_git_1(struct packed_git *p)
514 ssize_t read_result;
515 const unsigned hashsz = the_hash_algo->rawsz;
516
517 - if (!p->index_data) {
518 - struct multi_pack_index *m;
519 - const char *pack_name = pack_basename(p);
520 -
521 - for (m = the_repository->objects->multi_pack_index;
522 - m; m = m->next) {
523 - if (midx_contains_pack(m, pack_name))
524 - break;
525 - }
526 -
527 - if (!m && open_pack_index(p))
528 - return error("packfile %s index unavailable", p->pack_name);
529 - }
517 + if (open_pack_index(p))
518 + return error("packfile %s index unavailable", p->pack_name);
519
520 if (!pack_max_fds) {
521 unsigned int max_fds = get_max_fd_limit();
@@ -577,10 +566,6 @@ static int open_packed_git_1(struct packed_git *p)
566 " supported (try upgrading GIT to a newer version)",
567 p->pack_name, ntohl(hdr.hdr_version));
568
580 - /* Skip index checking if in multi-pack-index */
581 - if (!p->index_data)
582 - return 0;
583 -
569 /* Verify the pack matches its index. */
570 if (p->num_objects != ntohl(hdr.hdr_entries))
571 return error("packfile %s claims to have %"PRIu32" objects"
t/t5319-multi-pack-index.sh
+22 -2
@@ -117,7 +117,7 @@ test_expect_success 'write midx with one v2 pack' '
117
118 compare_results_with_midx "one v2 pack"
119
120 -test_expect_success 'corrupt idx not opened' '
120 +test_expect_success 'corrupt idx reports errors' '
121 idx=$(test-tool read-midx $objdir | grep "\.idx\$") &&
122 mv $objdir/pack/$idx backup-$idx &&
123 test_when_finished "mv backup-\$idx \$objdir/pack/\$idx" &&
@@ -128,7 +128,7 @@ test_expect_success 'corrupt idx not opened' '
128 test_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&
129
130 git -c core.multiPackIndex=true rev-list --objects --all 2>err &&
131 - test_must_be_empty err
131 + grep "index unavailable" err
132 '
133
134 test_expect_success 'add more objects' '
@@ -547,4 +547,24 @@ test_expect_success 'repack --batch-size=0 repacks everything' '
547 )
548 '
549
550 +test_expect_success 'load reverse index when missing .idx' '
551 + git init repo &&
552 + test_when_finished "rm -fr repo" &&
553 + (
554 + cd repo &&
555 +
556 + git config core.multiPackIndex true &&
557 +
558 + test_commit base &&
559 + git repack -ad &&
560 + git multi-pack-index write &&
561 +
562 + git rev-parse HEAD >tip &&
563 + idx=$(ls .git/objects/pack/pack-*.idx) &&
564 +
565 + mv $idx $idx.bak &&
566 + git cat-file --batch-check="%(objectsize:disk)" <tip
567 + )
568 +'
569 +
570 test_done