[PATCH 0/1] midx: prevent writing a bitmap with zero objects

[PATCH 0/1] midx: prevent writing a bitmap with zero objects

[Taylor Blau]

Here is a small patch I wrote after getting an alert that we tried to call
pack_pos_to_midx(..., 0) on a MIDX that contained zero objects.

The patch causes us to no longer write a bitmap for such a MIDX. More detailed
notes are within, and this is based on tb/midx-bitmap-corruption-fix. (The only
reason is that both this topic and that one add a new test to t5326, so merging
this one after that avoids a textual conflict).

Thanks in advance for your review.

Taylor Blau (1):
  midx: prevent writing a .bitmap without any objects

 midx.c                        |  9 +++++++++
 t/t5326-multi-pack-bitmaps.sh | 22 ++++++++++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.35.1.73.gccc5557600

[PATCH 1/1] midx: prevent writing a .bitmap without any objects

[Taylor Blau]

When trying to write a MIDX, we already prevent the case where there
weren't any packs present, and thus we would have written an empty MIDX.

But there is another "empty" case, which is more interesting, and we
don't yet handle. If we try to write a MIDX which has at least one pack,
but those packs together don't contain any objects, we will encounter a
BUG() when trying to use the bitmap corresponding to that MIDX, like so:

    $ git rev-parse HEAD | git pack-objects --revs --use-bitmap-index --stdout >/dev/null
    BUG: pack-revindex.c:394: pack_pos_to_midx: out-of-bounds object at 0

(note that in the above reproduction, both `--use-bitmap-index` and
`--stdout` are important, since without the former we won't even both to
load the .bitmap, and without the latter we wont attempt pack reuse).

The problem occurs when we try to discover the identity of the
preferred pack to determine which range if any of existing packs we can
reuse verbatim. This path is: `reuse_packfile_objects()` ->
`reuse_partial_packfile_from_bitmap()` -> `midx_preferred_pack()`.

    #4  0x000055555575401f in pack_pos_to_midx (m=0x555555997160, pos=0) at pack-revindex.c:394
    #5  0x00005555557502c8 in midx_preferred_pack (bitmap_git=0x55555599c280) at pack-bitmap.c:1431
    #6  0x000055555575036c in reuse_partial_packfile_from_bitmap (bitmap_git=0x55555599c280, packfile_out=0x5555559666b0 <reuse_packfile>,
        entries=0x5555559666b8 <reuse_packfile_objects>, reuse_out=0x5555559666c0 <reuse_packfile_bitmap>) at pack-bitmap.c:1452
    #7  0x00005555556041f6 in get_object_list_from_bitmap (revs=0x7fffffffcbf0) at builtin/pack-objects.c:3658
    #8  0x000055555560465c in get_object_list (ac=2, av=0x555555997050) at builtin/pack-objects.c:3765
    #9  0x0000555555605e4e in cmd_pack_objects (argc=0, argv=0x7fffffffe920, prefix=0x0) at builtin/pack-objects.c:4154

Since neither the .bitmap or MIDX stores the identity of the
preferred pack, we infer it by trying to load the first object in
pseudo-pack order, and then asking the MIDX which pack was chosen to
represent that object.

But this fails our bounds check, since there are zero objects in the
MIDX to begin with, which results in the BUG().

We could catch this more carefully in `midx_preferred_pack()`, but
signaling the absence of a preferred pack out to all of its callers is
somewhat awkward.

Instead, let's avoid writing a MIDX .bitmap without any objects
altogether. We catch this case in `write_midx_internal()`, and emit a
warning if the caller indicated they wanted to write a bitmap before
clearing out the relevant flags. If we somehow got to
write_midx_bitmap(), then we will call BUG(), but this should now be an
unreachable path.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c                        |  9 +++++++++
 t/t5326-multi-pack-bitmaps.sh | 22 ++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/midx.c b/midx.c
index 6e6cb0eb04..865170bad0 100644
--- a/midx.c
+++ b/midx.c
@@ -1077,6 +1077,9 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,
 	char *bitmap_name = xstrfmt("%s-%s.bitmap", midx_name, hash_to_hex(midx_hash));
 	int ret;
 
+	if (!ctx->entries_nr)
+		BUG("cannot write a bitmap without any objects");
+
 	if (flags & MIDX_WRITE_BITMAP_HASH_CACHE)
 		options |= BITMAP_OPT_HASH_CACHE;
 
@@ -1401,6 +1404,12 @@ static int write_midx_internal(const char *object_dir,
 		goto cleanup;
 	}
 
+	if (!ctx.entries_nr) {
+		if (flags & MIDX_WRITE_BITMAP)
+			warning(_("refusing to write multi-pack .bitmap without any objects"));
+		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
+	}
+
 	cf = init_chunkfile(f);
 
 	add_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 3c1ecc7e25..a5329a9f48 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -285,4 +285,26 @@ test_expect_success 'graceful fallback when missing reverse index' '
 	)
 '
 
+test_expect_success 'no .bitmap is written without any objects' '
+	rm -fr repo &&
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		empty="$(git pack-objects $objdir/pack/pack </dev/null)" &&
+		cat >packs <<-EOF &&
+		pack-$empty.idx
+		EOF
+
+		git multi-pack-index write --bitmap --stdin-packs \
+			<packs 2>err &&
+
+		grep "bitmap without any objects" err &&
+
+		test_path_is_file $midx &&
+		test_path_is_missing $midx-$(midx_checksum $objdir).bitmap
+	)
+'
+
 test_done
-- 
2.35.1.73.gccc5557600

Re: [PATCH 1/1] midx: prevent writing a .bitmap without any objects

[Derrick Stolee]

On 2/9/2022 2:26 PM, Taylor Blau wrote:
> When trying to write a MIDX, we already prevent the case where there
> weren't any packs present, and thus we would have written an empty MIDX.

> diff --git a/midx.c b/midx.c
> index 6e6cb0eb04..865170bad0 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1077,6 +1077,9 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,
>  	char *bitmap_name = xstrfmt("%s-%s.bitmap", midx_name, hash_to_hex(midx_hash));
>  	int ret;
>  
> +	if (!ctx->entries_nr)
> +		BUG("cannot write a bitmap without any objects");
> +
>  	if (flags & MIDX_WRITE_BITMAP_HASH_CACHE)
>  		options |= BITMAP_OPT_HASH_CACHE;
>  
> @@ -1401,6 +1404,12 @@ static int write_midx_internal(const char *object_dir,
>  		goto cleanup;
>  	}
>  
> +	if (!ctx.entries_nr) {
> +		if (flags & MIDX_WRITE_BITMAP)
> +			warning(_("refusing to write multi-pack .bitmap without any objects"));
> +		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
> +	}
> +

This patch looks good to me. The code change is simple and the
test is clear.

Thanks,
-Stolee