| Message ID | 55aa69de12c5f82a66836e829f915363cc73b421.1641320129.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | midx: prevent bitmap corruption when permuting pack order | expand |
Taylor Blau <me@ttaylorr.com> writes: > When a MIDX contains the new `RIDX` chunk, ensure that the reverse index > is read from it instead of the on-disk .rev file. Since we need to > encode the object order in the MIDX itself for correctness reasons, > there is no point in storing the same data again outside of the MIDX. > > So, this patch stops writing separate .rev files, and reads it out of > the MIDX itself. This is possible to do with relatively little new code, > since the format of the RIDX chunk is identical to the data in the .rev > file. In other words, we can implement this by pointing the > `revindex_data` field at the reverse index chunk of the MIDX instead of > the .rev file without any other changes. Ah, that's great. > Note that we have two knobs that are adjusted for the new tests: > GIT_TEST_MIDX_WRITE_REV and GIT_TEST_MIDX_READ_RIDX. The former controls > whether the MIDX .rev is written at all, and the latter controls whether > we read the MIDX's RIDX chunk. > > Both are necessary to ensure that the test added at the beginning of > this series continues to work. This is because we always need to write > the RIDX chunk in the MIDX in order to change its checksum, but we want > to make sure reading the existing .rev file still works (since the RIDX > chunk takes precedence by default). Could we disable the beginning-of-this-series test when the MIDX .rev is not written instead? Then, we can test what the user would actually experience (no reverse index in .midx, reverse index in .rev) instead of simulating it by skipping over the reverse index section in .midx. If we can't do that, then I would agree that the approach in this patch seems like the best approach.
On Mon, Jan 24, 2022 at 11:27:01AM -0800, Jonathan Tan wrote: > > Note that we have two knobs that are adjusted for the new tests: > > GIT_TEST_MIDX_WRITE_REV and GIT_TEST_MIDX_READ_RIDX. The former controls > > whether the MIDX .rev is written at all, and the latter controls whether > > we read the MIDX's RIDX chunk. > > > > Both are necessary to ensure that the test added at the beginning of > > this series continues to work. This is because we always need to write > > the RIDX chunk in the MIDX in order to change its checksum, but we want > > to make sure reading the existing .rev file still works (since the RIDX > > chunk takes precedence by default). > > Could we disable the beginning-of-this-series test when the MIDX .rev is > not written instead? Then, we can test what the user would actually > experience (no reverse index in .midx, reverse index in .rev) instead of > simulating it by skipping over the reverse index section in .midx. > > If we can't do that, then I would agree that the approach in this patch > seems like the best approach. I considered it, i.e., by having the two knobs be: - GIT_TEST_MIDX_WRITE_REV - GIT_TEST_MIDX_WRITE_RIDX where the pair (true, false) would correspond to what a corrupt repository would look like before this series. But I dislike that it allows the caller to alter the MIDX's checksum by controlling whether or not the chunk is written. So it's really looking at the same problem from two sides: do you make the RIDX chunk disappear by not reading it, or by never writing it in the first place? And although the latter is more "accurate", it did allow me to sidestep a lot of gory details like the ones I outlined in the second patch. I don't remember everything fully, since some time has passed since I originally wrote this, but I remember encountering some of the races where you'd read the old bitmap in the new object order, and other annoyances. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Mon, Jan 24, 2022 at 11:27:01AM -0800, Jonathan Tan wrote: > > > Note that we have two knobs that are adjusted for the new tests: > > > GIT_TEST_MIDX_WRITE_REV and GIT_TEST_MIDX_READ_RIDX. The former controls > > > whether the MIDX .rev is written at all, and the latter controls whether > > > we read the MIDX's RIDX chunk. > > > > > > Both are necessary to ensure that the test added at the beginning of > > > this series continues to work. This is because we always need to write > > > the RIDX chunk in the MIDX in order to change its checksum, but we want > > > to make sure reading the existing .rev file still works (since the RIDX > > > chunk takes precedence by default). > > > > Could we disable the beginning-of-this-series test when the MIDX .rev is > > not written instead? Then, we can test what the user would actually > > experience (no reverse index in .midx, reverse index in .rev) instead of > > simulating it by skipping over the reverse index section in .midx. > > > > If we can't do that, then I would agree that the approach in this patch > > seems like the best approach. > > I considered it, i.e., by having the two knobs be: > > - GIT_TEST_MIDX_WRITE_REV > - GIT_TEST_MIDX_WRITE_RIDX > > where the pair (true, false) would correspond to what a corrupt > repository would look like before this series. But I dislike that it > allows the caller to alter the MIDX's checksum by controlling whether or > not the chunk is written. I think that if the user really wanted to manipulate the checksum by using an environment variable for Git tests, they could just as easily do so by using an old version of Git. > So it's really looking at the same problem from two sides: do you make > the RIDX chunk disappear by not reading it, or by never writing it in > the first place? And although the latter is more "accurate", it did > allow me to sidestep a lot of gory details like the ones I outlined in > the second patch. > > I don't remember everything fully, since some time has passed since I > originally wrote this, but I remember encountering some of the races > where you'd read the old bitmap in the new object order, and other > annoyances. I think that feeling the need to avoid the gory details and races in test is an argument for also avoiding it in production (by not supporting external .rev files any more), but I've already explained my opinion on this and will leave it to others to decide. In any case, I still think that this patch set in its current form is worth merging - it is a strict improvement over what we had before (the race condition I'm worried about has been there before this patch set).
diff --git a/midx.c b/midx.c index d3179e9c02..9aba13b5b1 100644 --- a/midx.c +++ b/midx.c @@ -162,6 +162,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local pair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets); + if (git_env_bool("GIT_TEST_MIDX_READ_RIDX", 1)) + pair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex); + m->num_objects = ntohl(m->chunk_oid_fanout[255]); CALLOC_ARRAY(m->pack_names, m->num_packs); @@ -1429,7 +1432,8 @@ static int write_midx_internal(const char *object_dir, finalize_hashfile(f, midx_hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM); free_chunkfile(cf); - if (flags & MIDX_WRITE_REV_INDEX) + if (flags & MIDX_WRITE_REV_INDEX && + git_env_bool("GIT_TEST_MIDX_WRITE_REV", 0)) write_midx_reverse_index(midx_name.buf, midx_hash, &ctx); if (flags & MIDX_WRITE_BITMAP) { if (write_midx_bitmap(midx_name.buf, midx_hash, &ctx, diff --git a/midx.h b/midx.h index b7d79a515c..22e8e53288 100644 --- a/midx.h +++ b/midx.h @@ -36,6 +36,7 @@ struct multi_pack_index { const unsigned char *chunk_oid_lookup; const unsigned char *chunk_object_offsets; const unsigned char *chunk_large_offsets; + const unsigned char *chunk_revindex; const char **pack_names; struct packed_git **packs; diff --git a/pack-revindex.c b/pack-revindex.c index bd15ebad03..08dc160167 100644 --- a/pack-revindex.c +++ b/pack-revindex.c @@ -298,9 +298,26 @@ int load_midx_revindex(struct multi_pack_index *m) { struct strbuf revindex_name = STRBUF_INIT; int ret; + if (m->revindex_data) return 0; + if (m->chunk_revindex) { + /* + * If the MIDX `m` has a `RIDX` chunk, then use its contents for + * the reverse index instead of trying to load a separate `.rev` + * file. + * + * Note that we do *not* set `m->revindex_map` here, since we do + * not want to accidentally call munmap() in the middle of the + * MIDX. + */ + trace2_data_string("load_midx_revindex", the_repository, + "source", "midx"); + m->revindex_data = (const uint32_t *)m->chunk_revindex; + return 0; + } + trace2_data_string("load_midx_revindex", the_repository, "source", "rev"); diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh index 77b5f46a03..365d990ce3 100644 --- a/t/lib-bitmap.sh +++ b/t/lib-bitmap.sh @@ -290,7 +290,7 @@ test_rev_exists () { } midx_bitmap_core () { - rev_kind="${1:-rev}" + rev_kind="${1:-midx}" setup_bitmap_history @@ -434,7 +434,7 @@ midx_bitmap_core () { } midx_bitmap_partial_tests () { - rev_kind="${1:-rev}" + rev_kind="${1:-midx}" test_expect_success 'setup partial bitmaps' ' test_commit packed && diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh index 100ac90d15..c0924074c4 100755 --- a/t/t5326-multi-pack-bitmaps.sh +++ b/t/t5326-multi-pack-bitmaps.sh @@ -9,6 +9,12 @@ test_description='exercise basic multi-pack bitmap functionality' GIT_TEST_MULTI_PACK_INDEX=0 GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 +# This test exercise multi-pack bitmap functionality where the object order is +# stored and read from a special chunk within the MIDX, so use the default +# behavior here. +sane_unset GIT_TEST_MIDX_WRITE_REV +sane_unset GIT_TEST_MIDX_READ_RIDX + midx_bitmap_core bitmap_reuse_tests() { diff --git a/t/t5327-multi-pack-bitmaps-rev.sh b/t/t5327-multi-pack-bitmaps-rev.sh new file mode 100755 index 0000000000..d30ba632c8 --- /dev/null +++ b/t/t5327-multi-pack-bitmaps-rev.sh @@ -0,0 +1,23 @@ +#!/bin/sh + +test_description='exercise basic multi-pack bitmap functionality (.rev files)' + +. ./test-lib.sh +. "${TEST_DIRECTORY}/lib-bitmap.sh" + +# We'll be writing our own midx and bitmaps, so avoid getting confused by the +# automatic ones. +GIT_TEST_MULTI_PACK_INDEX=0 +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 + +# Unlike t5326, this test exercise multi-pack bitmap functionality where the +# object order is stored in a separate .rev file. +GIT_TEST_MIDX_WRITE_REV=1 +GIT_TEST_MIDX_READ_RIDX=0 +export GIT_TEST_MIDX_WRITE_REV +export GIT_TEST_MIDX_READ_RIDX + +midx_bitmap_core rev +midx_bitmap_partial_tests rev + +test_done diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 4693f8dc2b..02a6633a16 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -311,16 +311,13 @@ test_expect_success 'cleans up MIDX when appropriate' ' checksum=$(midx_checksum $objdir) && test_path_is_file $midx && test_path_is_file $midx-$checksum.bitmap && - test_path_is_file $midx-$checksum.rev && test_commit repack-3 && GIT_TEST_MULTI_PACK_INDEX=0 git repack -Adb --write-midx && test_path_is_file $midx && test_path_is_missing $midx-$checksum.bitmap && - test_path_is_missing $midx-$checksum.rev && test_path_is_file $midx-$(midx_checksum $objdir).bitmap && - test_path_is_file $midx-$(midx_checksum $objdir).rev && test_commit repack-4 && GIT_TEST_MULTI_PACK_INDEX=0 git repack -Adb && @@ -353,7 +350,6 @@ test_expect_success '--write-midx with preferred bitmap tips' ' test_line_count = 1 before && rm -fr $midx-$(midx_checksum $objdir).bitmap && - rm -fr $midx-$(midx_checksum $objdir).rev && rm -fr $midx && # instead of constructing the snapshot ourselves (c.f., the test
When a MIDX contains the new `RIDX` chunk, ensure that the reverse index is read from it instead of the on-disk .rev file. Since we need to encode the object order in the MIDX itself for correctness reasons, there is no point in storing the same data again outside of the MIDX. So, this patch stops writing separate .rev files, and reads it out of the MIDX itself. This is possible to do with relatively little new code, since the format of the RIDX chunk is identical to the data in the .rev file. In other words, we can implement this by pointing the `revindex_data` field at the reverse index chunk of the MIDX instead of the .rev file without any other changes. Note that we have two knobs that are adjusted for the new tests: GIT_TEST_MIDX_WRITE_REV and GIT_TEST_MIDX_READ_RIDX. The former controls whether the MIDX .rev is written at all, and the latter controls whether we read the MIDX's RIDX chunk. Both are necessary to ensure that the test added at the beginning of this series continues to work. This is because we always need to write the RIDX chunk in the MIDX in order to change its checksum, but we want to make sure reading the existing .rev file still works (since the RIDX chunk takes precedence by default). Arguably this isn't a very interesting mode to test, because the precedence rules mean that we'll always read the RIDX chunk over the .rev file. But it makes it impossible for a user to induce corruption in their repository by adjusting the test knobs (since if we had an either/or knob they could stop writing the RIDX chunk, allowing them to tweak the MIDX's object order without changing its checksum). Signed-off-by: Taylor Blau <me@ttaylorr.com> --- midx.c | 6 +++++- midx.h | 1 + pack-revindex.c | 17 +++++++++++++++++ t/lib-bitmap.sh | 4 ++-- t/t5326-multi-pack-bitmaps.sh | 6 ++++++ t/t5327-multi-pack-bitmaps-rev.sh | 23 +++++++++++++++++++++++ t/t7700-repack.sh | 4 ---- 7 files changed, 54 insertions(+), 7 deletions(-) create mode 100755 t/t5327-multi-pack-bitmaps-rev.sh