{"thread":{"id":"57394","subject":"[PATCH 0/1] midx: prevent writing a bitmap with zero objects","startedAt":"2022-02-09T19:29:09Z","lastAt":"2022-02-10T14:42:01Z","messageCount":3,"participants":["Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"448069","messageId":"cover.1644434802.git.me@ttaylorr.com","threadId":"57394","inReplyTo":null,"subject":"[PATCH 0/1] midx: prevent writing a bitmap with zero objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-09T19:26:45Z","receivedAt":"2022-02-09T19:29:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small patch I wrote after getting an alert that we tried to call\npack_pos_to_midx(..., 0) on a MIDX that contained zero objects.\n\nThe patch causes us to no longer write a bitmap for such a MIDX. More detailed\nnotes are within, and this is based on tb/midx-bitmap-corruption-fix. (The only\nreason is that both this topic and that one add a new test to t5326, so merging\nthis one after that avoids a textual conflict).\n\nThanks in advance for your review.\n\nTaylor Blau (1):\n midx: prevent writing a .bitmap without any objects\n\n midx.c | 9 +++++++++\n t/t5326-multi-pack-bitmaps.sh | 22 ++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\n-- \n2.35.1.73.gccc5557600\n"},{"id":"448070","messageId":"abc67d757cb6b244cf54b7b030985180ce134724.1644434802.git.me@ttaylorr.com","threadId":"57394","inReplyTo":"cover.1644434802.git.me@ttaylorr.com","subject":"[PATCH 1/1] midx: prevent writing a .bitmap without any objects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-09T19:26:47Z","receivedAt":"2022-02-09T19:29:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When trying to write a MIDX, we already prevent the case where there\nweren't any packs present, and thus we would have written an empty MIDX.\n\nBut there is another \"empty\" case, which is more interesting, and we\ndon't yet handle. If we try to write a MIDX which has at least one pack,\nbut those packs together don't contain any objects, we will encounter a\nBUG() when trying to use the bitmap corresponding to that MIDX, like so:\n\n $ git rev-parse HEAD | git pack-objects --revs --use-bitmap-index --stdout >/dev/null\n BUG: pack-revindex.c:394: pack_pos_to_midx: out-of-bounds object at 0\n\n(note that in the above reproduction, both `--use-bitmap-index` and\n`--stdout` are important, since without the former we won't even both to\nload the .bitmap, and without the latter we wont attempt pack reuse).\n\nThe problem occurs when we try to discover the identity of the\npreferred pack to determine which range if any of existing packs we can\nreuse verbatim. This path is: `reuse_packfile_objects()` ->\n`reuse_partial_packfile_from_bitmap()` -> `midx_preferred_pack()`.\n\n #4 0x000055555575401f in pack_pos_to_midx (m=0x555555997160, pos=0) at pack-revindex.c:394\n #5 0x00005555557502c8 in midx_preferred_pack (bitmap_git=0x55555599c280) at pack-bitmap.c:1431\n #6 0x000055555575036c in reuse_partial_packfile_from_bitmap (bitmap_git=0x55555599c280, packfile_out=0x5555559666b0 ,\n entries=0x5555559666b8 , reuse_out=0x5555559666c0 ) at pack-bitmap.c:1452\n #7 0x00005555556041f6 in get_object_list_from_bitmap (revs=0x7fffffffcbf0) at builtin/pack-objects.c:3658\n #8 0x000055555560465c in get_object_list (ac=2, av=0x555555997050) at builtin/pack-objects.c:3765\n #9 0x0000555555605e4e in cmd_pack_objects (argc=0, argv=0x7fffffffe920, prefix=0x0) at builtin/pack-objects.c:4154\n\nSince neither the .bitmap or MIDX stores the identity of the\npreferred pack, we infer it by trying to load the first object in\npseudo-pack order, and then asking the MIDX which pack was chosen to\nrepresent that object.\n\nBut this fails our bounds check, since there are zero objects in the\nMIDX to begin with, which results in the BUG().\n\nWe could catch this more carefully in `midx_preferred_pack()`, but\nsignaling the absence of a preferred pack out to all of its callers is\nsomewhat awkward.\n\nInstead, let's avoid writing a MIDX .bitmap without any objects\naltogether. We catch this case in `write_midx_internal()`, and emit a\nwarning if the caller indicated they wanted to write a bitmap before\nclearing out the relevant flags. If we somehow got to\nwrite_midx_bitmap(), then we will call BUG(), but this should now be an\nunreachable path.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 9 +++++++++\n t/t5326-multi-pack-bitmaps.sh | 22 ++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 6e6cb0eb04..865170bad0 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1077,6 +1077,9 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n \tint ret;\n \n+\tif (!ctx->entries_nr)\n+\t\tBUG(\"cannot write a bitmap without any objects\");\n+\n \tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n \t\toptions |= BITMAP_OPT_HASH_CACHE;\n \n@@ -1401,6 +1404,12 @@ static int write_midx_internal(const char *object_dir,\n \t\tgoto cleanup;\n \t}\n \n+\tif (!ctx.entries_nr) {\n+\t\tif (flags & MIDX_WRITE_BITMAP)\n+\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n+\t\tflags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n+\t}\n+\n \tcf = init_chunkfile(f);\n \n \tadd_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 3c1ecc7e25..a5329a9f48 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -285,4 +285,26 @@ test_expect_success 'graceful fallback when missing reverse index' '\n \t)\n '\n \n+test_expect_success 'no .bitmap is written without any objects' '\n+\trm -fr repo &&\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tempty=\"$(git pack-objects $objdir/pack/pack packs <<-EOF &&\n+\t\tpack-$empty.idx\n+\t\tEOF\n+\n+\t\tgit multi-pack-index write --bitmap --stdin-packs \\\n+\t\t\terr &&\n+\n+\t\tgrep \"bitmap without any objects\" err &&\n+\n+\t\ttest_path_is_file $midx &&\n+\t\ttest_path_is_missing $midx-$(midx_checksum $objdir).bitmap\n+\t)\n+'\n+\n test_done\n-- \n2.35.1.73.gccc5557600\n"},{"id":"448137","messageId":"748a2358-7bdf-5b28-09f9-5c4b266a3b52@gmail.com","threadId":"57394","inReplyTo":"abc67d757cb6b244cf54b7b030985180ce134724.1644434802.git.me@ttaylorr.com","subject":"Re: [PATCH 1/1] midx: prevent writing a .bitmap without any objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2022-02-10T14:41:51Z","receivedAt":"2022-02-10T14:42:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/9/2022 2:26 PM, Taylor Blau wrote:\n> When trying to write a MIDX, we already prevent the case where there\n> weren't any packs present, and thus we would have written an empty MIDX.\n\n> diff --git a/midx.c b/midx.c\n> index 6e6cb0eb04..865170bad0 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1077,6 +1077,9 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n> \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n> \tint ret;\n> \n> +\tif (!ctx->entries_nr)\n> +\t\tBUG(\"cannot write a bitmap without any objects\");\n> +\n> \tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n> \t\toptions |= BITMAP_OPT_HASH_CACHE;\n> \n> @@ -1401,6 +1404,12 @@ static int write_midx_internal(const char *object_dir,\n> \t\tgoto cleanup;\n> \t}\n> \n> +\tif (!ctx.entries_nr) {\n> +\t\tif (flags & MIDX_WRITE_BITMAP)\n> +\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n> +\t\tflags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n> +\t}\n> +\n\nThis patch looks good to me. The code change is simple and the\ntest is clear.\n\nThanks,\n-Stolee\n"}]}