diff mbox series

[v2,8/8] midx: read `RIDX` chunk when present

Message ID 993bfa8dd8480e74d64f657539b0c518ad155e5c.1639446906.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series midx: prevent bitmap corruption when permuting pack order | expand

Commit Message

Taylor Blau Dec. 14, 2021, 1:55 a.m. UTC 
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     |  5 +++++
 t/t5327-multi-pack-bitmaps-rev.sh | 23 +++++++++++++++++++++++
 t/t7700-repack.sh                 |  4 ----
 7 files changed, 53 insertions(+), 7 deletions(-)
 create mode 100755 t/t5327-multi-pack-bitmaps-rev.sh

Comments

Derrick Stolee Dec. 20, 2021, 6:42 p.m. UTC | #1 
On 12/13/2021 8:55 PM, Taylor Blau 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.

...

> +	if (git_env_bool("GIT_TEST_MIDX_READ_RIDX", 1))
> +		pair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);
> +

Skip reading if GIT_TEST_MIDX_READ_RIDX=0 (do read if true or unset). Good.

> -	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);

Only write if GIT_TEST_MIDX_WRITE_REV=1 (skip if false or unset). Good.

> +	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");

A natural way to do this.

>  
> diff --git a/t/lib-bitmap.sh b/t/lib-bitmap.sh
> index 0a35daf939..f5eaa9cf68 100644
> --- a/t/lib-bitmap.sh
> +++ b/t/lib-bitmap.sh
> @@ -291,7 +291,7 @@ test_rev_exists () {
>  }
>  
>  midx_bitmap_core () {
> -	rev_kind="${1:-rev}"
> +	rev_kind="${1:-midx}"
>  
>  	setup_bitmap_history
>  
> @@ -435,7 +435,7 @@ midx_bitmap_core () {
>  }
>  
>  midx_bitmap_partial_tests () {
> -	rev_kind="${1:-rev}"
> +	rev_kind="${1:-midx}"

And I'm glad we can just update the default here.

> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> index 100ac90d15..8c92acb0ce 100755
> --- a/t/t5326-multi-pack-bitmaps.sh
> +++ b/t/t5326-multi-pack-bitmaps.sh
> @@ -9,6 +9,11 @@ test_description='exercise basic multi-pack bitmap functionality'
>  GIT_TEST_MULTI_PACK_INDEX=0
>  GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
>  
> +GIT_TEST_MIDX_WRITE_REV=0
> +GIT_TEST_MIDX_READ_RIDX=1
> +export GIT_TEST_MIDX_WRITE_REV
> +export GIT_TEST_MIDX_READ_RIDX

Technically, we could unset these variables, right? ("...=")

> diff --git a/t/t5327-multi-pack-bitmaps-rev.sh b/t/t5327-multi-pack-bitmaps-rev.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

Nice that now we get the payoff of the test refactor. This new script exists
only to verify that we can still read the old .rev files, which is important
for compatibility.

Forgive me for "speaking out loud" throughout this patch. I don't see any
need for changes but it is the crux of making this series work.

Thanks,
-Stolee
Taylor Blau Jan. 4, 2022, 3:21 p.m. UTC | #2 
On Mon, Dec 20, 2021 at 01:42:24PM -0500, Derrick Stolee wrote:
> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> > index 100ac90d15..8c92acb0ce 100755
> > --- a/t/t5326-multi-pack-bitmaps.sh
> > +++ b/t/t5326-multi-pack-bitmaps.sh
> > @@ -9,6 +9,11 @@ test_description='exercise basic multi-pack bitmap functionality'
> >  GIT_TEST_MULTI_PACK_INDEX=0
> >  GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
> >
> > +GIT_TEST_MIDX_WRITE_REV=0
> > +GIT_TEST_MIDX_READ_RIDX=1
> > +export GIT_TEST_MIDX_WRITE_REV
> > +export GIT_TEST_MIDX_READ_RIDX
>
> Technically, we could unset these variables, right? ("...=")

We absolutely could, and I can't think of any reason not to (other than
these tests would be fragile with respect to the default values of these
special "GIT_TEST_" environment variables, but these are unlikely to
change ever).

> > diff --git a/t/t5327-multi-pack-bitmaps-rev.sh b/t/t5327-multi-pack-bitmaps-rev.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
>
> Nice that now we get the payoff of the test refactor. This new script exists
> only to verify that we can still read the old .rev files, which is important
> for compatibility.

Exactly. Though I certainly wavered on testing the legacy .rev behavior
as much as I did. But that had much more to do with how awkward the
tests came out. That behavior is important enough IMHO that it's worth
the awkwardness.

Thanks,
Taylor
diff mbox series

Patch

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 0a35daf939..f5eaa9cf68 100644
--- a/t/lib-bitmap.sh
+++ b/t/lib-bitmap.sh
@@ -291,7 +291,7 @@  test_rev_exists () {
 }
 
 midx_bitmap_core () {
-	rev_kind="${1:-rev}"
+	rev_kind="${1:-midx}"
 
 	setup_bitmap_history
 
@@ -435,7 +435,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..8c92acb0ce 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -9,6 +9,11 @@  test_description='exercise basic multi-pack bitmap functionality'
 GIT_TEST_MULTI_PACK_INDEX=0
 GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
 
+GIT_TEST_MIDX_WRITE_REV=0
+GIT_TEST_MIDX_READ_RIDX=1
+export GIT_TEST_MIDX_WRITE_REV
+export 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 0260ad6f0e..3ee56cefd3 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