Skip to content

Commit

Permalink
midx: prevent writing a .bitmap without any objects
Browse files Browse the repository at this point in the history
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
ttaylorr authored and gitster committed Feb 9, 2022
1 parent dcc0cd0 commit eb57277
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 0 deletions.
9 changes: 9 additions & 0 deletions midx.c
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,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;

Expand Down Expand Up @@ -1385,6 +1388,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,
Expand Down
22 changes: 22 additions & 0 deletions t/t5326-multi-pack-bitmaps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -395,4 +395,26 @@ test_expect_success 'hash-cache values are propagated from pack bitmaps' '
)
'

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

0 comments on commit eb57277

Please sign in to comment.