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@xxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
packfile.c | 19 ++-----------------
t/t5319-multi-pack-index.sh | 24 ++++++++++++++++++++++--
2 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/packfile.c b/packfile.c
index 0929ebe4fc..8d7f37a5f6 100644
--- a/packfile.c
+++ b/packfile.c
@@ -514,19 +514,8 @@ static int open_packed_git_1(struct packed_git *p)
ssize_t read_result;
const unsigned hashsz = the_hash_algo->rawsz;
- if (!p->index_data) {
- struct multi_pack_index *m;
- const char *pack_name = pack_basename(p);
-
- for (m = the_repository->objects->multi_pack_index;
- m; m = m->next) {
- if (midx_contains_pack(m, pack_name))
- break;
- }
-
- if (!m && open_pack_index(p))
- return error("packfile %s index unavailable", p->pack_name);
- }
+ if (open_pack_index(p))
+ return error("packfile %s index unavailable", p->pack_name);
if (!pack_max_fds) {
unsigned int max_fds = get_max_fd_limit();
@@ -567,10 +556,6 @@ static int open_packed_git_1(struct packed_git *p)
" supported (try upgrading GIT to a newer version)",
p->pack_name, ntohl(hdr.hdr_version));
- /* Skip index checking if in multi-pack-index */
- if (!p->index_data)
- return 0;
-
/* Verify the pack matches its index. */
if (p->num_objects != ntohl(hdr.hdr_entries))
return error("packfile %s claims to have %"PRIu32" objects"
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index ace469c95c..d4607daec1 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -138,7 +138,7 @@ test_expect_success 'write midx with one v2 pack' '
compare_results_with_midx "one v2 pack"
-test_expect_success 'corrupt idx not opened' '
+test_expect_success 'corrupt idx reports errors' '
idx=$(test-tool read-midx $objdir | grep "\.idx\$") &&
mv $objdir/pack/$idx backup-$idx &&
test_when_finished "mv backup-\$idx \$objdir/pack/\$idx" &&
@@ -149,7 +149,7 @@ test_expect_success 'corrupt idx not opened' '
test_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&
git -c core.multiPackIndex=true rev-list --objects --all 2>err &&
- test_must_be_empty err
+ grep "index unavailable" err
'
test_expect_success 'add more objects' '
@@ -755,4 +755,24 @@ test_expect_success 'repack --batch-size=<large> repacks everything' '
)
'
+test_expect_success 'load reverse index when missing .idx' '
+ git init repo &&
+ test_when_finished "rm -fr repo" &&
+ (
+ cd repo &&
+
+ git config core.multiPackIndex true &&
+
+ test_commit base &&
+ git repack -ad &&
+ git multi-pack-index write &&
+
+ git rev-parse HEAD >tip &&
+ idx=$(ls .git/objects/pack/pack-*.idx) &&
+
+ mv $idx $idx.bak &&
+ git cat-file --batch-check="%(objectsize:disk)" <tip
+ )
+'
+
test_done
--
2.29.2.368.ge1806d1bdc
