patchmidx: prevent writing a bitmap with zero objects

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(+)

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(+)

On 2/9/2022 2:26 PM, Taylor Blau wrote:

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

Thanks, -Stolee