{"thread":{"id":"61610","subject":"[PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","startedAt":"2024-06-09T15:27:38Z","lastAt":"2024-06-11T17:31:24Z","messageCount":15,"participants":["Taylor Blau","Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496737","messageId":"4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com","threadId":"61610","inReplyTo":null,"subject":"[PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-09T15:27:35Z","receivedAt":"2024-06-09T15:27:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When performing multi-pack reuse, reuse_partial_packfile_from_bitmap()\nis responsible for generating an array of bitmapped_pack structs from\nwhich to perform reuse.\n\nIn the multi-pack case, we loop over the MIDXs packs and copy the result\nof calling `nth_bitmapped_pack()` to construct the list of reusable\npaths.\n\nBut we may also want to do pack-reuse over a single pack, either because\nwe only had one pack to perform reuse over (in the case of single-pack\nbitmaps), or because we explicitly asked to do single pack reuse even\nwith a MIDX[^1].\n\nWhen this is the case, the array we generate of reusable packs contains\nonly a single element, which is either (a) the pack attached to the\nsingle-pack bitmap, or (b) the MIDX's preferred pack.\n\nIn 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\nfunction and stopped assigning the pack_int_id field when reusing only\nthe MIDX's preferred pack. This results in an uninitialized read down in\ntry_partial_reuse() like so:\n\n    ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value\n    #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8\n    #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8\n    #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3\n    #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3\n    #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\n    #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\n    #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\n\nwhich happens when try_partial_reuse() tries to call\nmidx_pair_to_pack_pos() when it tries to reject cross-pack deltas.\n\nAvoid the uninitialized read by ensuring that the pack_int_id field is\nset in the single-pack reuse case by setting it to either the MIDX\npreferred pack's pack_int_id, or '0', in the case of single-pack\nbitmaps.  In the latter case, we never read the pack_int_id field, so\nthe choice of '0' is arbitrary.\n\n[^1]: This can happen for a couple of reasons, either because the\n  repository is configured with 'pack.allowPackReuse=(true|single)', or\n  because the MIDX was generated prior to the introduction of the BTMP\n  chunk, which contains information necessary to perform multi-pack\n  reuse.\n\nReported-by: Kyle Lippincott <spectral@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fe8e8a51d3..8b9a2c698f 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2073,6 +2073,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n \t} else {\n \t\tstruct packed_git *pack;\n+\t\tuint32_t pack_int_id;\n\n \t\tif (bitmap_is_midx(bitmap_git)) {\n \t\t\tuint32_t preferred_pack_pos;\n@@ -2083,12 +2084,21 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\t}\n\n \t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n+\t\t\tpack_int_id = preferred_pack_pos;\n \t\t} else {\n \t\t\tpack = bitmap_git->pack;\n+\t\t\t/*\n+\t\t\t * Any value for 'pack_int_id' will do here. When we\n+\t\t\t * process the pack via try_partial_reuse(), we won't\n+\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n+\t\t\t * bitmap.\n+\t\t\t */\n+\t\t\tpack_int_id = 0;\n \t\t}\n\n \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n \t\tpacks[packs_nr].p = pack;\n+\t\tpacks[packs_nr].pack_int_id = pack_int_id;\n \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n \t\tpacks[packs_nr].bitmap_pos = 0;\n\n--\n2.45.2.445.g1b76f06508\n"},{"id":"496746","messageId":"ZmaVYnmgyAr0vapK@tanuki","threadId":"61610","inReplyTo":"4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-10T05:55:46Z","receivedAt":"2024-06-10T05:55:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 09, 2024 at 11:27:35AM -0400, Taylor Blau wrote:\n> In 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n> 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\n> function and stopped assigning the pack_int_id field when reusing only\n> the MIDX's preferred pack. This results in an uninitialized read down in\n> try_partial_reuse() like so:\n\nI feel like I'm blind, but I cannot see how the patch changed what we do\nwith `pack_int_id`. It's not mentioned a single time in the diff, so how\ndid it have the effect of not setting it anymore?\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index fe8e8a51d3..8b9a2c698f 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -2073,6 +2073,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n>  \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n>  \t} else {\n>  \t\tstruct packed_git *pack;\n> +\t\tuint32_t pack_int_id;\n> \n>  \t\tif (bitmap_is_midx(bitmap_git)) {\n>  \t\t\tuint32_t preferred_pack_pos;\n> @@ -2083,12 +2084,21 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n>  \t\t\t}\n> \n>  \t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n> +\t\t\tpack_int_id = preferred_pack_pos;\n>  \t\t} else {\n>  \t\t\tpack = bitmap_git->pack;\n> +\t\t\t/*\n> +\t\t\t * Any value for 'pack_int_id' will do here. When we\n> +\t\t\t * process the pack via try_partial_reuse(), we won't\n> +\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n> +\t\t\t * bitmap.\n> +\t\t\t */\n> +\t\t\tpack_int_id = 0;\n>  \t\t}\n> \n>  \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n>  \t\tpacks[packs_nr].p = pack;\n> +\t\tpacks[packs_nr].pack_int_id = pack_int_id;\n>  \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n>  \t\tpacks[packs_nr].bitmap_pos = 0;\n\nOkay, the patch looks trivial enough. I was wondering whether we might\nwant to `memset(&packs[packs_nr], 0, sizeof(packs[packs_nr]))` as it\nfeels rather easy to miss initialization of one of the members. But on\nthe other hand, this might also cause us to paper over bugs if we did\nthat.\n\nPatrick\n"},{"id":"496776","messageId":"ZmcUclCErqwyad9D@nand.local","threadId":"61610","inReplyTo":"ZmaVYnmgyAr0vapK@tanuki","subject":"Re: [PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-10T14:57:54Z","receivedAt":"2024-06-10T14:57:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 10, 2024 at 07:55:46AM +0200, Patrick Steinhardt wrote:\n> On Sun, Jun 09, 2024 at 11:27:35AM -0400, Taylor Blau wrote:\n> > In 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n> > 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\n> > function and stopped assigning the pack_int_id field when reusing only\n> > the MIDX's preferred pack. This results in an uninitialized read down in\n> > try_partial_reuse() like so:\n>\n> I feel like I'm blind, but I cannot see how the patch changed what we do\n> with `pack_int_id`. It's not mentioned a single time in the diff, so how\n> did it have the effect of not setting it anymore?\n\nIt's because prior to 795006fff4, we handled reusing a single pack from\na MIDX differently than in the post-image of that commit. Prior to\n795006fff4, the loop looked like:\n\n    if (bitmap_is_midx(bitmap_git)) {\n            for (i = 0; i < bitmap_git->midx->num_packs; i++) {\n                    struct bitmapped_pack pack;\n                    if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n                          /* ... */\n                          return;\n                    }\n                    if (!pack.bitmap_nr)\n                          continue;\n                    if (!multi_pack_reuse && pack.bitmap_pos)\n                          continue;\n\n                    ALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n                    memcpy(&packs[packs_nr++], &pack, sizeof(pack));\n            }\n    }\n\nSince nth_bitmapped_pack() fills out the pack_int_id field, we got it\nautomatically since we just memcpy()'d the result of\nnth_bitmapped_pack() into our array.\n\nIn the single pack bitmap case, we don't need to initialize the\npack_int_id field because we never read it, hence the lack of MSan\nfailures in that mode.\n\nBut since 795006fff4 combined these two single pack cases (that is,\nsingle-pack bitmaps, and reusing only a single pack out of a MIDX\nbitmap) into one, 795006fff4 neglected to initialize the pack_int_id\nfield, causing this issue.\n\nThanks,\nTaylor\n"},{"id":"496796","messageId":"cover.1718050244.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com","subject":"[PATCH v2 0/3] midx: various brown paper bag fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-10T20:10:46Z","receivedAt":"2024-06-10T20:10:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series fixes a couple of brown paper bag bugs in the MIDX/bitmap\ncode.\n\nThis started as a single patch to address the MSan issue pointing to\nuninitialized memory being used as a pack ID when trying to reject\ncross-pack deltas during verbatim pack reuse.\n\nThe latter two patches of this series address that issue. The middle\npatch squashes the bug and hardens against further regressions in that\narea with a test. The last patch further hardens us by ensuring that the\n\"pack\" input to midx_key_to_pack_pos() is bounded by the number of packs\nin the MIDX.\n\nThe first patch in this series fixes another bug I noticed while writing\nthe test in the second patch which bisects to my d6a8c58675\n(midx-write.c: support reading an existing MIDX with `packs_to_include`,\n2024-05-29).\n\nI would like to address the more fundamental issues that are described\nin the first patch, but I think the reality is that doing so is much\nmore involved than a short series during week 6 of the release cycle\nwarrants. So instead, the first patch is a revert of d6a8c58675 (plus a\ntest to demonstrate the issue with it) which buys us some time to teach\nthe MIDX code how to handle not every pack being carried forward from an\nexisting MIDX.\n\nThanks in advance for your review, and sorry for these embarrassing\nbugs.\n\nTaylor Blau (3):\n  midx-write.c: do not read existing MIDX with `packs_to_include`\n  pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n  pack-revindex.c: guard against out-of-bounds pack lookups\n\n midx-write.c                  | 42 ++++++++++++++++++++++++++---------\n pack-bitmap.c                 | 10 +++++++++\n pack-revindex.c               |  3 +++\n t/t5326-multi-pack-bitmaps.sh | 30 +++++++++++++++++++++++++\n t/t5332-multi-pack-reuse.sh   | 26 ++++++++++++++++++++++\n 5 files changed, 100 insertions(+), 11 deletions(-)\n\nRange-diff against v1:\n1:  4aceb9233e ! 1:  0bdf925366 pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n    @@ Metadata\n     Author: Taylor Blau <me@ttaylorr.com>\n     \n      ## Commit message ##\n    -    pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n    -\n    -    When performing multi-pack reuse, reuse_partial_packfile_from_bitmap()\n    -    is responsible for generating an array of bitmapped_pack structs from\n    -    which to perform reuse.\n    -\n    -    In the multi-pack case, we loop over the MIDXs packs and copy the result\n    -    of calling `nth_bitmapped_pack()` to construct the list of reusable\n    -    paths.\n    -\n    -    But we may also want to do pack-reuse over a single pack, either because\n    -    we only had one pack to perform reuse over (in the case of single-pack\n    -    bitmaps), or because we explicitly asked to do single pack reuse even\n    -    with a MIDX[^1].\n    -\n    -    When this is the case, the array we generate of reusable packs contains\n    -    only a single element, which is either (a) the pack attached to the\n    -    single-pack bitmap, or (b) the MIDX's preferred pack.\n    -\n    -    In 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n    -    2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\n    -    function and stopped assigning the pack_int_id field when reusing only\n    -    the MIDX's preferred pack. This results in an uninitialized read down in\n    -    try_partial_reuse() like so:\n    -\n    -        ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value\n    -        #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8\n    -        #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8\n    -        #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3\n    -        #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3\n    -        #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\n    -        #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\n    -        #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\n    -\n    -    which happens when try_partial_reuse() tries to call\n    -    midx_pair_to_pack_pos() when it tries to reject cross-pack deltas.\n    -\n    -    Avoid the uninitialized read by ensuring that the pack_int_id field is\n    -    set in the single-pack reuse case by setting it to either the MIDX\n    -    preferred pack's pack_int_id, or '0', in the case of single-pack\n    -    bitmaps.  In the latter case, we never read the pack_int_id field, so\n    -    the choice of '0' is arbitrary.\n    -\n    -    [^1]: This can happen for a couple of reasons, either because the\n    -      repository is configured with 'pack.allowPackReuse=(true|single)', or\n    -      because the MIDX was generated prior to the introduction of the BTMP\n    -      chunk, which contains information necessary to perform multi-pack\n    -      reuse.\n    -\n    -    Reported-by: Kyle Lippincott <spectral@google.com>\n    +    midx-write.c: do not read existing MIDX with `packs_to_include`\n    +\n    +    Commit d6a8c58675 (midx-write.c: support reading an existing MIDX with\n    +    `packs_to_include`, 2024-05-29) changed the MIDX generation machinery to\n    +    support reading from an existing MIDX when writing a new one.\n    +\n    +    Unfortunately, the rest of the MIDX generation machinery is not prepared\n    +    to deal with such a change. For instance, the function responsible for\n    +    adding to the object ID fanout table from a MIDX source\n    +    (midx_fanout_add_midx_fanout()) will gladly add objects from an existing\n    +    MIDX for some fanout level regardless of whether or not those objects\n    +    came from packs that are to be included in the subsequent MIDX write.\n    +\n    +    This results in broken pseudo-pack object order (leading to incorrect\n    +    object traversal results) and segmentation faults, like so (generated by\n    +    running the added test prior to the changes in midx-write.c):\n    +\n    +        #0  0x000055ee31393f47 in midx_pack_order (ctx=0x7ffdde205c70) at midx-write.c:590\n    +        #1  0x000055ee31395a69 in write_midx_internal (object_dir=0x55ee32570440 \".git/objects\",\n    +            packs_to_include=0x7ffdde205e20, packs_to_drop=0x0, preferred_pack_name=0x0,\n    +            refs_snapshot=0x0, flags=15) at midx-write.c:1171\n    +        #2  0x000055ee31395f38 in write_midx_file_only (object_dir=0x55ee32570440 \".git/objects\",\n    +            packs_to_include=0x7ffdde205e20, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15)\n    +            at midx-write.c:1274\n    +        [...]\n    +\n    +    In stack frame #0, the code on midx-write.c:590 is using the new pack ID\n    +    corresponding to some object which was added from the existing MIDX.\n    +    Importantly, the pack from which that object was selected in the\n    +    existing MIDX does not appear in the new MIDX as it was excluded via\n    +    `--stdin-packs`.\n    +\n    +    In this instance, the pack in question had pack ID \"1\" in the existing\n    +    MIDX, but since it was excluded from the new MIDX, we never filled in\n    +    that entry in the pack_perm table, resulting in:\n    +\n    +        (gdb) p *ctx->pack_perm@2\n    +        $1 = {0, 1515870810}\n    +\n    +    Which is what causes the segfault above when we try and read:\n    +\n    +        struct pack_info *pack = &ctx->info[ctx->pack_perm[i]];\n    +        if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)\n    +            pack->bitmap_pos = 0;\n    +\n    +    Fundamentally, we should be able to read information from an existing\n    +    MIDX when generating a new one. But in practice the midx-write.c code\n    +    assumes that we won't run into issues like the above with incongruent\n    +    pack IDs, and often makes those assumptions in extremely subtle and\n    +    fragile ways.\n    +\n    +    Instead, let's avoid reading from an existing MIDX altogether, and stick\n    +    with the pre-d6a8c58675 implementation. Harden against any regressions\n    +    in this area by adding a test which demonstrates these issues.\n    +\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n    - ## pack-bitmap.c ##\n    -@@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n    - \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n    - \t} else {\n    - \t\tstruct packed_git *pack;\n    -+\t\tuint32_t pack_int_id;\n    - \n    - \t\tif (bitmap_is_midx(bitmap_git)) {\n    - \t\t\tuint32_t preferred_pack_pos;\n    -@@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n    - \t\t\t}\n    - \n    - \t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n    -+\t\t\tpack_int_id = preferred_pack_pos;\n    - \t\t} else {\n    - \t\t\tpack = bitmap_git->pack;\n    -+\t\t\t/*\n    -+\t\t\t * Any value for 'pack_int_id' will do here. When we\n    -+\t\t\t * process the pack via try_partial_reuse(), we won't\n    -+\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n    -+\t\t\t * bitmap.\n    -+\t\t\t */\n    -+\t\t\tpack_int_id = 0;\n    - \t\t}\n    - \n    - \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n    - \t\tpacks[packs_nr].p = pack;\n    -+\t\tpacks[packs_nr].pack_int_id = pack_int_id;\n    - \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n    - \t\tpacks[packs_nr].bitmap_pos = 0;\n    + ## midx-write.c ##\n    +@@ midx-write.c: struct write_midx_context {\n    + };\n      \n    + static int should_include_pack(const struct write_midx_context *ctx,\n    +-\t\t\t       const char *file_name,\n    +-\t\t\t       int exclude_from_midx)\n    ++\t\t\t       const char *file_name)\n    + {\n    +-\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n    ++\t/*\n    ++\t * Note that at most one of ctx->m and ctx->to_include are set,\n    ++\t * so we are testing midx_contains_pack() and\n    ++\t * string_list_has_string() independently (guarded by the\n    ++\t * appropriate NULL checks).\n    ++\t *\n    ++\t * We could support passing to_include while reusing an existing\n    ++\t * MIDX, but don't currently since the reuse process drags\n    ++\t * forward all packs from an existing MIDX (without checking\n    ++\t * whether or not they appear in the to_include list).\n    ++\t *\n    ++\t * If we added support for that, these next two conditional\n    ++\t * should be performed independently (likely checking\n    ++\t * to_include before the existing MIDX).\n    ++\t */\n    ++\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n    + \t\treturn 0;\n    +-\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n    +-\t\t\t\t\t\t       file_name))\n    ++\telse if (ctx->to_include &&\n    ++\t\t !string_list_has_string(ctx->to_include, file_name))\n    + \t\treturn 0;\n    + \treturn 1;\n    + }\n    +@@ midx-write.c: static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n    + \tif (ends_with(file_name, \".idx\")) {\n    + \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n    + \n    +-\t\tif (!should_include_pack(ctx, file_name, 1))\n    ++\t\tif (!should_include_pack(ctx, file_name))\n    + \t\t\treturn;\n    + \n    + \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n    +@@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n    + \tuint32_t i;\n    + \n    + \tfor (i = 0; i < ctx->m->num_packs; i++) {\n    +-\t\tif (!should_include_pack(ctx, ctx->m->pack_names[i], 0))\n    +-\t\t\tcontinue;\n    +-\n    + \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n    + \n    + \t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n    +@@ midx-write.c: static int write_midx_internal(const char *object_dir,\n    + \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n    + \t\t\t  midx_name.buf);\n    + \n    +-\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n    ++\tif (!packs_to_include) {\n    ++\t\t/*\n    ++\t\t * Only reference an existing MIDX when not filtering which\n    ++\t\t * packs to include, since all packs and objects are copied\n    ++\t\t * blindly from an existing MIDX if one is present.\n    ++\t\t */\n    ++\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n    ++\t}\n    ++\n    + \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n    + \t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n    + \t\tctx.m = NULL;\n    +@@ midx-write.c: static int write_midx_internal(const char *object_dir,\n    + \tctx.nr = 0;\n    + \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n    + \tctx.info = NULL;\n    +-\tctx.to_include = packs_to_include;\n    + \tALLOC_ARRAY(ctx.info, ctx.alloc);\n    + \n    + \tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n    +@@ midx-write.c: static int write_midx_internal(const char *object_dir,\n    + \telse\n    + \t\tctx.progress = NULL;\n    + \n    ++\tctx.to_include = packs_to_include;\n    ++\n    + \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n    + \tstop_progress(&ctx.progress);\n    + \n    +\n    + ## t/t5326-multi-pack-bitmaps.sh ##\n    +@@ t/t5326-multi-pack-bitmaps.sh: do\n    + \t'\n    + done\n    + \n    ++test_expect_success 'remove one packfile between MIDX bitmap writes' '\n    ++\tgit init remove-pack-between-writes &&\n    ++\t(\n    ++\t\tcd remove-pack-between-writes &&\n    ++\n    ++\t\ttest_commit A &&\n    ++\t\ttest_commit B &&\n    ++\t\ttest_commit C &&\n    ++\n    ++\t\t# Create packs with the prefix \"pack-A\", \"pack-B\",\n    ++\t\t# \"pack-C\" to impose a lexicographic order on these\n    ++\t\t# packs so the pack being removed is always from the\n    ++\t\t# middle.\n    ++\t\tpackdir=.git/objects/pack &&\n    ++\t\tA=\"$(echo A | git pack-objects $packdir/pack-A --revs)\" &&\n    ++\t\tB=\"$(echo B | git pack-objects $packdir/pack-B --revs)\" &&\n    ++\t\tC=\"$(echo C | git pack-objects $packdir/pack-C --revs)\" &&\n    ++\n    ++\t\tgit multi-pack-index write --bitmap &&\n    ++\n    ++\t\tcat >in <<-EOF &&\n    ++\t\tpack-A-$A.idx\n    ++\t\tpack-C-$C.idx\n    ++\t\tEOF\n    ++\t\tgit multi-pack-index write --bitmap --stdin-packs <in &&\n    ++\n    ++\t\tgit rev-list --test-bitmap HEAD\n    ++\t)\n    ++'\n    ++\n    + test_done\n-:  ---------- > 2:  a3c28f1202 pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n-:  ---------- > 3:  dadcf96c06 pack-revindex.c: guard against out-of-bounds pack lookups\n\nbase-commit: 1b76f065085811104b5f4adff001956d7e5c5d1c\n-- \n2.45.2.447.g6b4ffca7ec.dirty\n"},{"id":"496797","messageId":"0bdf925366daea7ed229883272da79dbcf7023b3.1718050244.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"cover.1718050244.git.me@ttaylorr.com","subject":"[PATCH v2 1/3] midx-write.c: do not read existing MIDX with `packs_to_include`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-10T20:10:49Z","receivedAt":"2024-06-10T20:10:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Commit d6a8c58675 (midx-write.c: support reading an existing MIDX with\n`packs_to_include`, 2024-05-29) changed the MIDX generation machinery to\nsupport reading from an existing MIDX when writing a new one.\n\nUnfortunately, the rest of the MIDX generation machinery is not prepared\nto deal with such a change. For instance, the function responsible for\nadding to the object ID fanout table from a MIDX source\n(midx_fanout_add_midx_fanout()) will gladly add objects from an existing\nMIDX for some fanout level regardless of whether or not those objects\ncame from packs that are to be included in the subsequent MIDX write.\n\nThis results in broken pseudo-pack object order (leading to incorrect\nobject traversal results) and segmentation faults, like so (generated by\nrunning the added test prior to the changes in midx-write.c):\n\n    #0  0x000055ee31393f47 in midx_pack_order (ctx=0x7ffdde205c70) at midx-write.c:590\n    #1  0x000055ee31395a69 in write_midx_internal (object_dir=0x55ee32570440 \".git/objects\",\n        packs_to_include=0x7ffdde205e20, packs_to_drop=0x0, preferred_pack_name=0x0,\n        refs_snapshot=0x0, flags=15) at midx-write.c:1171\n    #2  0x000055ee31395f38 in write_midx_file_only (object_dir=0x55ee32570440 \".git/objects\",\n        packs_to_include=0x7ffdde205e20, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15)\n        at midx-write.c:1274\n    [...]\n\nIn stack frame #0, the code on midx-write.c:590 is using the new pack ID\ncorresponding to some object which was added from the existing MIDX.\nImportantly, the pack from which that object was selected in the\nexisting MIDX does not appear in the new MIDX as it was excluded via\n`--stdin-packs`.\n\nIn this instance, the pack in question had pack ID \"1\" in the existing\nMIDX, but since it was excluded from the new MIDX, we never filled in\nthat entry in the pack_perm table, resulting in:\n\n    (gdb) p *ctx->pack_perm@2\n    $1 = {0, 1515870810}\n\nWhich is what causes the segfault above when we try and read:\n\n    struct pack_info *pack = &ctx->info[ctx->pack_perm[i]];\n    if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)\n        pack->bitmap_pos = 0;\n\nFundamentally, we should be able to read information from an existing\nMIDX when generating a new one. But in practice the midx-write.c code\nassumes that we won't run into issues like the above with incongruent\npack IDs, and often makes those assumptions in extremely subtle and\nfragile ways.\n\nInstead, let's avoid reading from an existing MIDX altogether, and stick\nwith the pre-d6a8c58675 implementation. Harden against any regressions\nin this area by adding a test which demonstrates these issues.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx-write.c                  | 42 ++++++++++++++++++++++++++---------\n t/t5326-multi-pack-bitmaps.sh | 30 +++++++++++++++++++++++++\n 2 files changed, 61 insertions(+), 11 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 55a6b63bac..0125aa4dc9 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -101,13 +101,27 @@ struct write_midx_context {\n };\n \n static int should_include_pack(const struct write_midx_context *ctx,\n-\t\t\t       const char *file_name,\n-\t\t\t       int exclude_from_midx)\n+\t\t\t       const char *file_name)\n {\n-\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n+\t/*\n+\t * Note that at most one of ctx->m and ctx->to_include are set,\n+\t * so we are testing midx_contains_pack() and\n+\t * string_list_has_string() independently (guarded by the\n+\t * appropriate NULL checks).\n+\t *\n+\t * We could support passing to_include while reusing an existing\n+\t * MIDX, but don't currently since the reuse process drags\n+\t * forward all packs from an existing MIDX (without checking\n+\t * whether or not they appear in the to_include list).\n+\t *\n+\t * If we added support for that, these next two conditional\n+\t * should be performed independently (likely checking\n+\t * to_include before the existing MIDX).\n+\t */\n+\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n \t\treturn 0;\n-\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n-\t\t\t\t\t\t       file_name))\n+\telse if (ctx->to_include &&\n+\t\t !string_list_has_string(ctx->to_include, file_name))\n \t\treturn 0;\n \treturn 1;\n }\n@@ -121,7 +135,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \tif (ends_with(file_name, \".idx\")) {\n \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n \n-\t\tif (!should_include_pack(ctx, file_name, 1))\n+\t\tif (!should_include_pack(ctx, file_name))\n \t\t\treturn;\n \n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n@@ -880,9 +894,6 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \tuint32_t i;\n \n \tfor (i = 0; i < ctx->m->num_packs; i++) {\n-\t\tif (!should_include_pack(ctx, ctx->m->pack_names[i], 0))\n-\t\t\tcontinue;\n-\n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \n \t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n@@ -937,7 +948,15 @@ static int write_midx_internal(const char *object_dir,\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n \t\t\t  midx_name.buf);\n \n-\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n+\tif (!packs_to_include) {\n+\t\t/*\n+\t\t * Only reference an existing MIDX when not filtering which\n+\t\t * packs to include, since all packs and objects are copied\n+\t\t * blindly from an existing MIDX if one is present.\n+\t\t */\n+\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n+\t}\n+\n \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n \t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n \t\tctx.m = NULL;\n@@ -946,7 +965,6 @@ static int write_midx_internal(const char *object_dir,\n \tctx.nr = 0;\n \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n \tctx.info = NULL;\n-\tctx.to_include = packs_to_include;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n \n \tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n@@ -963,6 +981,8 @@ static int write_midx_internal(const char *object_dir,\n \telse\n \t\tctx.progress = NULL;\n \n+\tctx.to_include = packs_to_include;\n+\n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex cc7220b6c0..916da389b6 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -551,4 +551,34 @@ do\n \t'\n done\n \n+test_expect_success 'remove one packfile between MIDX bitmap writes' '\n+\tgit init remove-pack-between-writes &&\n+\t(\n+\t\tcd remove-pack-between-writes &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\t# Create packs with the prefix \"pack-A\", \"pack-B\",\n+\t\t# \"pack-C\" to impose a lexicographic order on these\n+\t\t# packs so the pack being removed is always from the\n+\t\t# middle.\n+\t\tpackdir=.git/objects/pack &&\n+\t\tA=\"$(echo A | git pack-objects $packdir/pack-A --revs)\" &&\n+\t\tB=\"$(echo B | git pack-objects $packdir/pack-B --revs)\" &&\n+\t\tC=\"$(echo C | git pack-objects $packdir/pack-C --revs)\" &&\n+\n+\t\tgit multi-pack-index write --bitmap &&\n+\n+\t\tcat >in <<-EOF &&\n+\t\tpack-A-$A.idx\n+\t\tpack-C-$C.idx\n+\t\tEOF\n+\t\tgit multi-pack-index write --bitmap --stdin-packs <in &&\n+\n+\t\tgit rev-list --test-bitmap HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.45.2.447.g6b4ffca7ec.dirty\n\n"},{"id":"496798","messageId":"a3c28f12020712818060bd50d1483507c9b11556.1718050244.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"cover.1718050244.git.me@ttaylorr.com","subject":"[PATCH v2 2/3] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-10T20:10:53Z","receivedAt":"2024-06-10T20:10:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When performing multi-pack reuse, reuse_partial_packfile_from_bitmap()\nis responsible for generating an array of bitmapped_pack structs from\nwhich to perform reuse.\n\nIn the multi-pack case, we loop over the MIDXs packs and copy the result\nof calling `nth_bitmapped_pack()` to construct the list of reusable\npaths.\n\nBut we may also want to do pack-reuse over a single pack, either because\nwe only had one pack to perform reuse over (in the case of single-pack\nbitmaps), or because we explicitly asked to do single pack reuse even\nwith a MIDX[^1].\n\nWhen this is the case, the array we generate of reusable packs contains\nonly a single element, which is either (a) the pack attached to the\nsingle-pack bitmap, or (b) the MIDX's preferred pack.\n\nIn 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\nfunction and stopped assigning the pack_int_id field when reusing only\nthe MIDX's preferred pack. This results in an uninitialized read down in\ntry_partial_reuse() like so:\n\n    ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value\n    #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8\n    #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8\n    #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3\n    #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3\n    #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\n    #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\n    #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\n\nwhich happens when try_partial_reuse() tries to call\nmidx_pair_to_pack_pos() when it tries to reject cross-pack deltas.\n\nAvoid the uninitialized read by ensuring that the pack_int_id field is\nset in the single-pack reuse case by setting it to either the MIDX\npreferred pack's pack_int_id, or '0', in the case of single-pack\nbitmaps.  In the latter case, we never read the pack_int_id field, so\nthe choice of '0' is arbitrary.\n\nGuard against further regressions in this area by adding a test which\nensures that we do not throw out deltas from the preferred pack as\n\"cross-pack\" due to an uninitialized pack_int_id.\n\n[^1]: This can happen for a couple of reasons, either because the\n  repository is configured with 'pack.allowPackReuse=(true|single)', or\n  because the MIDX was generated prior to the introduction of the BTMP\n  chunk, which contains information necessary to perform multi-pack\n  reuse.\n\nReported-by: Kyle Lippincott <spectral@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c               | 10 ++++++++++\n t/t5332-multi-pack-reuse.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fe8e8a51d3..8b9a2c698f 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2073,6 +2073,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n \t} else {\n \t\tstruct packed_git *pack;\n+\t\tuint32_t pack_int_id;\n \n \t\tif (bitmap_is_midx(bitmap_git)) {\n \t\t\tuint32_t preferred_pack_pos;\n@@ -2083,12 +2084,21 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\t}\n \n \t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n+\t\t\tpack_int_id = preferred_pack_pos;\n \t\t} else {\n \t\t\tpack = bitmap_git->pack;\n+\t\t\t/*\n+\t\t\t * Any value for 'pack_int_id' will do here. When we\n+\t\t\t * process the pack via try_partial_reuse(), we won't\n+\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n+\t\t\t * bitmap.\n+\t\t\t */\n+\t\t\tpack_int_id = 0;\n \t\t}\n \n \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n \t\tpacks[packs_nr].p = pack;\n+\t\tpacks[packs_nr].pack_int_id = pack_int_id;\n \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n \t\tpacks[packs_nr].bitmap_pos = 0;\n \ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 3c20738bce..ed823f37bc 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -204,4 +204,30 @@ test_expect_success 'omit delta from uninteresting base (cross pack)' '\n \ttest_pack_objects_reused_all $(($objects_nr - 1)) $packs_nr\n '\n \n+test_expect_success 'non-omitted delta in MIDX preferred pack' '\n+\ttest_config pack.allowPackReuse single &&\n+\n+\tcat >p1.objects <<-EOF &&\n+\t$(git rev-parse $base)\n+\t^$(git rev-parse $delta^)\n+\tEOF\n+\tcat >p2.objects <<-EOF &&\n+\t$(git rev-parse F)\n+\tEOF\n+\n+\tp1=\"$(git pack-objects --revs $packdir/pack <p1.objects)\" &&\n+\tp2=\"$(git pack-objects --revs $packdir/pack <p2.objects)\" &&\n+\n+\tcat >in <<-EOF &&\n+\tpack-$p1.idx\n+\tpack-$p2.idx\n+\tEOF\n+\tgit multi-pack-index write --bitmap --stdin-packs \\\n+\t\t--preferred-pack=pack-$p1.pack <in &&\n+\n+\tgit show-index <$packdir/pack-$p1.idx >expect &&\n+\n+\ttest_pack_objects_reused_all $(wc -l <expect) 1\n+'\n+\n test_done\n-- \n2.45.2.447.g6b4ffca7ec.dirty\n\n"},{"id":"496799","messageId":"dadcf96c06ffaf29abefc9adfc1450f399089549.1718050244.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"cover.1718050244.git.me@ttaylorr.com","subject":"[PATCH v2 3/3] pack-revindex.c: guard against out-of-bounds pack lookups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-10T20:10:56Z","receivedAt":"2024-06-10T20:10:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function midx_key_to_pack_pos() is a helper function used by\nmidx_to_pack_pos() and midx_pair_to_pack_pos() to translate a (pack,\noffset) tuple into a position into the MIDX pseudo-pack order.\n\nEnsure that the pack ID given to midx_pair_to_pack_pos() is bounded by\nthe number of packs within the MIDX to prevent, for instance,\nuninitialized memory from being used as a pack ID.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-revindex.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex fc63aa76a2..93ffca7731 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -527,6 +527,9 @@ static int midx_key_to_pack_pos(struct multi_pack_index *m,\n {\n \tuint32_t *found;\n \n+\tif (key->pack >= m->num_packs)\n+\t\tBUG(\"MIDX pack lookup out of bounds (%\"PRIu32\" >= %\"PRIu32\")\",\n+\t\t    key->pack, m->num_packs);\n \t/*\n \t * The preferred pack sorts first, so determine its identifier by\n \t * looking at the first object in pseudo-pack order.\n-- \n2.45.2.447.g6b4ffca7ec.dirty\n"},{"id":"496841","messageId":"ZmgG3nCmprSN-mkh@tanuki","threadId":"61610","inReplyTo":"ZmcUclCErqwyad9D@nand.local","subject":"Re: [PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-11T08:12:14Z","receivedAt":"2024-06-11T08:12:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 10, 2024 at 10:57:54AM -0400, Taylor Blau wrote:\n> On Mon, Jun 10, 2024 at 07:55:46AM +0200, Patrick Steinhardt wrote:\n> > On Sun, Jun 09, 2024 at 11:27:35AM -0400, Taylor Blau wrote:\n> > > In 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n> > > 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\n> > > function and stopped assigning the pack_int_id field when reusing only\n> > > the MIDX's preferred pack. This results in an uninitialized read down in\n> > > try_partial_reuse() like so:\n> >\n> > I feel like I'm blind, but I cannot see how the patch changed what we do\n> > with `pack_int_id`. It's not mentioned a single time in the diff, so how\n> > did it have the effect of not setting it anymore?\n> \n> It's because prior to 795006fff4, we handled reusing a single pack from\n> a MIDX differently than in the post-image of that commit. Prior to\n> 795006fff4, the loop looked like:\n> \n>     if (bitmap_is_midx(bitmap_git)) {\n>             for (i = 0; i < bitmap_git->midx->num_packs; i++) {\n>                     struct bitmapped_pack pack;\n>                     if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n>                           /* ... */\n>                           return;\n>                     }\n>                     if (!pack.bitmap_nr)\n>                           continue;\n>                     if (!multi_pack_reuse && pack.bitmap_pos)\n>                           continue;\n> \n>                     ALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n>                     memcpy(&packs[packs_nr++], &pack, sizeof(pack));\n>             }\n>     }\n> \n> Since nth_bitmapped_pack() fills out the pack_int_id field, we got it\n> automatically since we just memcpy()'d the result of\n> nth_bitmapped_pack() into our array.\n> \n> In the single pack bitmap case, we don't need to initialize the\n> pack_int_id field because we never read it, hence the lack of MSan\n> failures in that mode.\n> \n> But since 795006fff4 combined these two single pack cases (that is,\n> single-pack bitmaps, and reusing only a single pack out of a MIDX\n> bitmap) into one, 795006fff4 neglected to initialize the pack_int_id\n> field, causing this issue.\n\nMakes sense, thanks for the explanation!\n\nPatrick\n"},{"id":"496857","messageId":"20240611091102.GL3248245@coredump.intra.peff.net","threadId":"61610","inReplyTo":"a3c28f12020712818060bd50d1483507c9b11556.1718050244.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/3] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-11T09:11:02Z","receivedAt":"2024-06-11T09:11:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 10, 2024 at 04:10:53PM -0400, Taylor Blau wrote:\n\n> Avoid the uninitialized read by ensuring that the pack_int_id field is\n> set in the single-pack reuse case by setting it to either the MIDX\n> preferred pack's pack_int_id, or '0', in the case of single-pack\n> bitmaps.  In the latter case, we never read the pack_int_id field, so\n> the choice of '0' is arbitrary.\n\nCould we set it to some sentinel value for the single-pack case? If we\nset it to \"-1\", then the BUG() added in patch 3 would trigger if we did\naccidentally try to feed it to the midx code. Assuming you do not have\n2^32-1 packs, of course. ;)\n\nI am OK either way, though. And the rest of the patch looked good.\n\n-Peff\n"},{"id":"496929","messageId":"xmqqy17bwgxd.fsf@gitster.g","threadId":"61610","inReplyTo":"20240611091102.GL3248245@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/3] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T17:03:10Z","receivedAt":"2024-06-11T17:03:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jun 10, 2024 at 04:10:53PM -0400, Taylor Blau wrote:\n>\n>> Avoid the uninitialized read by ensuring that the pack_int_id field is\n>> set in the single-pack reuse case by setting it to either the MIDX\n>> preferred pack's pack_int_id, or '0', in the case of single-pack\n>> bitmaps.  In the latter case, we never read the pack_int_id field, so\n>> the choice of '0' is arbitrary.\n>\n> Could we set it to some sentinel value for the single-pack case? If we\n> set it to \"-1\", then the BUG() added in patch 3 would trigger if we did\n> accidentally try to feed it to the midx code. Assuming you do not have\n> 2^32-1 packs, of course. ;)\n\nYeah, I had exactly the same reaction.\n"},{"id":"496930","messageId":"cover.1718126886.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com","subject":"[PATCH v2 0/3] midx: various brown paper bag fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-11T17:28:07Z","receivedAt":"2024-06-11T17:28:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series fixes a couple of brown paper bag bugs in the MIDX/bitmap\ncode.\n\nThis version is basically identical to the previous round, except that\nwe use the sentinel value \"-1\" as the 'pack_int_id' when reusing from a\nsingle (non-MIDX'd) pack. This is a \"garbage in, garbage out\" measure to\nensure that we notice any regressions here loudly.\n\nThanks in advance for your review!\n\nTaylor Blau (3):\n  midx-write.c: do not read existing MIDX with `packs_to_include`\n  pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n  pack-revindex.c: guard against out-of-bounds pack lookups\n\n midx-write.c                  | 42 ++++++++++++++++++++++++++---------\n pack-bitmap.c                 | 13 +++++++++++\n pack-revindex.c               |  3 +++\n t/t5326-multi-pack-bitmaps.sh | 30 +++++++++++++++++++++++++\n t/t5332-multi-pack-reuse.sh   | 26 ++++++++++++++++++++++\n 5 files changed, 103 insertions(+), 11 deletions(-)\n\nRange-diff against v1:\n1:  4aceb9233e ! 1:  0bdf925366 pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n    @@ Metadata\n     Author: Taylor Blau <me@ttaylorr.com>\n     \n      ## Commit message ##\n    -    pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n    -\n    -    When performing multi-pack reuse, reuse_partial_packfile_from_bitmap()\n    -    is responsible for generating an array of bitmapped_pack structs from\n    -    which to perform reuse.\n    -\n    -    In the multi-pack case, we loop over the MIDXs packs and copy the result\n    -    of calling `nth_bitmapped_pack()` to construct the list of reusable\n    -    paths.\n    -\n    -    But we may also want to do pack-reuse over a single pack, either because\n    -    we only had one pack to perform reuse over (in the case of single-pack\n    -    bitmaps), or because we explicitly asked to do single pack reuse even\n    -    with a MIDX[^1].\n    -\n    -    When this is the case, the array we generate of reusable packs contains\n    -    only a single element, which is either (a) the pack attached to the\n    -    single-pack bitmap, or (b) the MIDX's preferred pack.\n    -\n    -    In 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n    -    2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\n    -    function and stopped assigning the pack_int_id field when reusing only\n    -    the MIDX's preferred pack. This results in an uninitialized read down in\n    -    try_partial_reuse() like so:\n    -\n    -        ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value\n    -        #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8\n    -        #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8\n    -        #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3\n    -        #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3\n    -        #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\n    -        #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\n    -        #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\n    -\n    -    which happens when try_partial_reuse() tries to call\n    -    midx_pair_to_pack_pos() when it tries to reject cross-pack deltas.\n    -\n    -    Avoid the uninitialized read by ensuring that the pack_int_id field is\n    -    set in the single-pack reuse case by setting it to either the MIDX\n    -    preferred pack's pack_int_id, or '0', in the case of single-pack\n    -    bitmaps.  In the latter case, we never read the pack_int_id field, so\n    -    the choice of '0' is arbitrary.\n    -\n    -    [^1]: This can happen for a couple of reasons, either because the\n    -      repository is configured with 'pack.allowPackReuse=(true|single)', or\n    -      because the MIDX was generated prior to the introduction of the BTMP\n    -      chunk, which contains information necessary to perform multi-pack\n    -      reuse.\n    -\n    -    Reported-by: Kyle Lippincott <spectral@google.com>\n    +    midx-write.c: do not read existing MIDX with `packs_to_include`\n    +\n    +    Commit d6a8c58675 (midx-write.c: support reading an existing MIDX with\n    +    `packs_to_include`, 2024-05-29) changed the MIDX generation machinery to\n    +    support reading from an existing MIDX when writing a new one.\n    +\n    +    Unfortunately, the rest of the MIDX generation machinery is not prepared\n    +    to deal with such a change. For instance, the function responsible for\n    +    adding to the object ID fanout table from a MIDX source\n    +    (midx_fanout_add_midx_fanout()) will gladly add objects from an existing\n    +    MIDX for some fanout level regardless of whether or not those objects\n    +    came from packs that are to be included in the subsequent MIDX write.\n    +\n    +    This results in broken pseudo-pack object order (leading to incorrect\n    +    object traversal results) and segmentation faults, like so (generated by\n    +    running the added test prior to the changes in midx-write.c):\n    +\n    +        #0  0x000055ee31393f47 in midx_pack_order (ctx=0x7ffdde205c70) at midx-write.c:590\n    +        #1  0x000055ee31395a69 in write_midx_internal (object_dir=0x55ee32570440 \".git/objects\",\n    +            packs_to_include=0x7ffdde205e20, packs_to_drop=0x0, preferred_pack_name=0x0,\n    +            refs_snapshot=0x0, flags=15) at midx-write.c:1171\n    +        #2  0x000055ee31395f38 in write_midx_file_only (object_dir=0x55ee32570440 \".git/objects\",\n    +            packs_to_include=0x7ffdde205e20, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15)\n    +            at midx-write.c:1274\n    +        [...]\n    +\n    +    In stack frame #0, the code on midx-write.c:590 is using the new pack ID\n    +    corresponding to some object which was added from the existing MIDX.\n    +    Importantly, the pack from which that object was selected in the\n    +    existing MIDX does not appear in the new MIDX as it was excluded via\n    +    `--stdin-packs`.\n    +\n    +    In this instance, the pack in question had pack ID \"1\" in the existing\n    +    MIDX, but since it was excluded from the new MIDX, we never filled in\n    +    that entry in the pack_perm table, resulting in:\n    +\n    +        (gdb) p *ctx->pack_perm@2\n    +        $1 = {0, 1515870810}\n    +\n    +    Which is what causes the segfault above when we try and read:\n    +\n    +        struct pack_info *pack = &ctx->info[ctx->pack_perm[i]];\n    +        if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)\n    +            pack->bitmap_pos = 0;\n    +\n    +    Fundamentally, we should be able to read information from an existing\n    +    MIDX when generating a new one. But in practice the midx-write.c code\n    +    assumes that we won't run into issues like the above with incongruent\n    +    pack IDs, and often makes those assumptions in extremely subtle and\n    +    fragile ways.\n    +\n    +    Instead, let's avoid reading from an existing MIDX altogether, and stick\n    +    with the pre-d6a8c58675 implementation. Harden against any regressions\n    +    in this area by adding a test which demonstrates these issues.\n    +\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n    - ## pack-bitmap.c ##\n    -@@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n    - \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n    - \t} else {\n    - \t\tstruct packed_git *pack;\n    -+\t\tuint32_t pack_int_id;\n    - \n    - \t\tif (bitmap_is_midx(bitmap_git)) {\n    - \t\t\tuint32_t preferred_pack_pos;\n    -@@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n    - \t\t\t}\n    - \n    - \t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n    -+\t\t\tpack_int_id = preferred_pack_pos;\n    - \t\t} else {\n    - \t\t\tpack = bitmap_git->pack;\n    -+\t\t\t/*\n    -+\t\t\t * Any value for 'pack_int_id' will do here. When we\n    -+\t\t\t * process the pack via try_partial_reuse(), we won't\n    -+\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n    -+\t\t\t * bitmap.\n    -+\t\t\t */\n    -+\t\t\tpack_int_id = 0;\n    - \t\t}\n    - \n    - \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n    - \t\tpacks[packs_nr].p = pack;\n    -+\t\tpacks[packs_nr].pack_int_id = pack_int_id;\n    - \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n    - \t\tpacks[packs_nr].bitmap_pos = 0;\n    + ## midx-write.c ##\n    +@@ midx-write.c: struct write_midx_context {\n    + };\n      \n    + static int should_include_pack(const struct write_midx_context *ctx,\n    +-\t\t\t       const char *file_name,\n    +-\t\t\t       int exclude_from_midx)\n    ++\t\t\t       const char *file_name)\n    + {\n    +-\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n    ++\t/*\n    ++\t * Note that at most one of ctx->m and ctx->to_include are set,\n    ++\t * so we are testing midx_contains_pack() and\n    ++\t * string_list_has_string() independently (guarded by the\n    ++\t * appropriate NULL checks).\n    ++\t *\n    ++\t * We could support passing to_include while reusing an existing\n    ++\t * MIDX, but don't currently since the reuse process drags\n    ++\t * forward all packs from an existing MIDX (without checking\n    ++\t * whether or not they appear in the to_include list).\n    ++\t *\n    ++\t * If we added support for that, these next two conditional\n    ++\t * should be performed independently (likely checking\n    ++\t * to_include before the existing MIDX).\n    ++\t */\n    ++\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n    + \t\treturn 0;\n    +-\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n    +-\t\t\t\t\t\t       file_name))\n    ++\telse if (ctx->to_include &&\n    ++\t\t !string_list_has_string(ctx->to_include, file_name))\n    + \t\treturn 0;\n    + \treturn 1;\n    + }\n    +@@ midx-write.c: static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n    + \tif (ends_with(file_name, \".idx\")) {\n    + \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n    + \n    +-\t\tif (!should_include_pack(ctx, file_name, 1))\n    ++\t\tif (!should_include_pack(ctx, file_name))\n    + \t\t\treturn;\n    + \n    + \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n    +@@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n    + \tuint32_t i;\n    + \n    + \tfor (i = 0; i < ctx->m->num_packs; i++) {\n    +-\t\tif (!should_include_pack(ctx, ctx->m->pack_names[i], 0))\n    +-\t\t\tcontinue;\n    +-\n    + \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n    + \n    + \t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n    +@@ midx-write.c: static int write_midx_internal(const char *object_dir,\n    + \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n    + \t\t\t  midx_name.buf);\n    + \n    +-\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n    ++\tif (!packs_to_include) {\n    ++\t\t/*\n    ++\t\t * Only reference an existing MIDX when not filtering which\n    ++\t\t * packs to include, since all packs and objects are copied\n    ++\t\t * blindly from an existing MIDX if one is present.\n    ++\t\t */\n    ++\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n    ++\t}\n    ++\n    + \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n    + \t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n    + \t\tctx.m = NULL;\n    +@@ midx-write.c: static int write_midx_internal(const char *object_dir,\n    + \tctx.nr = 0;\n    + \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n    + \tctx.info = NULL;\n    +-\tctx.to_include = packs_to_include;\n    + \tALLOC_ARRAY(ctx.info, ctx.alloc);\n    + \n    + \tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n    +@@ midx-write.c: static int write_midx_internal(const char *object_dir,\n    + \telse\n    + \t\tctx.progress = NULL;\n    + \n    ++\tctx.to_include = packs_to_include;\n    ++\n    + \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n    + \tstop_progress(&ctx.progress);\n    + \n    +\n    + ## t/t5326-multi-pack-bitmaps.sh ##\n    +@@ t/t5326-multi-pack-bitmaps.sh: do\n    + \t'\n    + done\n    + \n    ++test_expect_success 'remove one packfile between MIDX bitmap writes' '\n    ++\tgit init remove-pack-between-writes &&\n    ++\t(\n    ++\t\tcd remove-pack-between-writes &&\n    ++\n    ++\t\ttest_commit A &&\n    ++\t\ttest_commit B &&\n    ++\t\ttest_commit C &&\n    ++\n    ++\t\t# Create packs with the prefix \"pack-A\", \"pack-B\",\n    ++\t\t# \"pack-C\" to impose a lexicographic order on these\n    ++\t\t# packs so the pack being removed is always from the\n    ++\t\t# middle.\n    ++\t\tpackdir=.git/objects/pack &&\n    ++\t\tA=\"$(echo A | git pack-objects $packdir/pack-A --revs)\" &&\n    ++\t\tB=\"$(echo B | git pack-objects $packdir/pack-B --revs)\" &&\n    ++\t\tC=\"$(echo C | git pack-objects $packdir/pack-C --revs)\" &&\n    ++\n    ++\t\tgit multi-pack-index write --bitmap &&\n    ++\n    ++\t\tcat >in <<-EOF &&\n    ++\t\tpack-A-$A.idx\n    ++\t\tpack-C-$C.idx\n    ++\t\tEOF\n    ++\t\tgit multi-pack-index write --bitmap --stdin-packs <in &&\n    ++\n    ++\t\tgit rev-list --test-bitmap HEAD\n    ++\t)\n    ++'\n    ++\n    + test_done\n-:  ---------- > 2:  4b006f44a5 pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n-:  ---------- > 3:  06de4005f1 pack-revindex.c: guard against out-of-bounds pack lookups\n\nbase-commit: 1b76f065085811104b5f4adff001956d7e5c5d1c\n-- \n2.45.2.448.g06de4005f1\n"},{"id":"496931","messageId":"0bdf925366daea7ed229883272da79dbcf7023b3.1718126886.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"cover.1718126886.git.me@ttaylorr.com","subject":"[PATCH v2 1/3] midx-write.c: do not read existing MIDX with `packs_to_include`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-11T17:28:17Z","receivedAt":"2024-06-11T17:28:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Commit d6a8c58675 (midx-write.c: support reading an existing MIDX with\n`packs_to_include`, 2024-05-29) changed the MIDX generation machinery to\nsupport reading from an existing MIDX when writing a new one.\n\nUnfortunately, the rest of the MIDX generation machinery is not prepared\nto deal with such a change. For instance, the function responsible for\nadding to the object ID fanout table from a MIDX source\n(midx_fanout_add_midx_fanout()) will gladly add objects from an existing\nMIDX for some fanout level regardless of whether or not those objects\ncame from packs that are to be included in the subsequent MIDX write.\n\nThis results in broken pseudo-pack object order (leading to incorrect\nobject traversal results) and segmentation faults, like so (generated by\nrunning the added test prior to the changes in midx-write.c):\n\n    #0  0x000055ee31393f47 in midx_pack_order (ctx=0x7ffdde205c70) at midx-write.c:590\n    #1  0x000055ee31395a69 in write_midx_internal (object_dir=0x55ee32570440 \".git/objects\",\n        packs_to_include=0x7ffdde205e20, packs_to_drop=0x0, preferred_pack_name=0x0,\n        refs_snapshot=0x0, flags=15) at midx-write.c:1171\n    #2  0x000055ee31395f38 in write_midx_file_only (object_dir=0x55ee32570440 \".git/objects\",\n        packs_to_include=0x7ffdde205e20, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15)\n        at midx-write.c:1274\n    [...]\n\nIn stack frame #0, the code on midx-write.c:590 is using the new pack ID\ncorresponding to some object which was added from the existing MIDX.\nImportantly, the pack from which that object was selected in the\nexisting MIDX does not appear in the new MIDX as it was excluded via\n`--stdin-packs`.\n\nIn this instance, the pack in question had pack ID \"1\" in the existing\nMIDX, but since it was excluded from the new MIDX, we never filled in\nthat entry in the pack_perm table, resulting in:\n\n    (gdb) p *ctx->pack_perm@2\n    $1 = {0, 1515870810}\n\nWhich is what causes the segfault above when we try and read:\n\n    struct pack_info *pack = &ctx->info[ctx->pack_perm[i]];\n    if (pack->bitmap_pos == BITMAP_POS_UNKNOWN)\n        pack->bitmap_pos = 0;\n\nFundamentally, we should be able to read information from an existing\nMIDX when generating a new one. But in practice the midx-write.c code\nassumes that we won't run into issues like the above with incongruent\npack IDs, and often makes those assumptions in extremely subtle and\nfragile ways.\n\nInstead, let's avoid reading from an existing MIDX altogether, and stick\nwith the pre-d6a8c58675 implementation. Harden against any regressions\nin this area by adding a test which demonstrates these issues.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx-write.c                  | 42 ++++++++++++++++++++++++++---------\n t/t5326-multi-pack-bitmaps.sh | 30 +++++++++++++++++++++++++\n 2 files changed, 61 insertions(+), 11 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 55a6b63bac..0125aa4dc9 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -101,13 +101,27 @@ struct write_midx_context {\n };\n \n static int should_include_pack(const struct write_midx_context *ctx,\n-\t\t\t       const char *file_name,\n-\t\t\t       int exclude_from_midx)\n+\t\t\t       const char *file_name)\n {\n-\tif (exclude_from_midx && ctx->m && midx_contains_pack(ctx->m, file_name))\n+\t/*\n+\t * Note that at most one of ctx->m and ctx->to_include are set,\n+\t * so we are testing midx_contains_pack() and\n+\t * string_list_has_string() independently (guarded by the\n+\t * appropriate NULL checks).\n+\t *\n+\t * We could support passing to_include while reusing an existing\n+\t * MIDX, but don't currently since the reuse process drags\n+\t * forward all packs from an existing MIDX (without checking\n+\t * whether or not they appear in the to_include list).\n+\t *\n+\t * If we added support for that, these next two conditional\n+\t * should be performed independently (likely checking\n+\t * to_include before the existing MIDX).\n+\t */\n+\tif (ctx->m && midx_contains_pack(ctx->m, file_name))\n \t\treturn 0;\n-\tif (ctx->to_include && !string_list_has_string(ctx->to_include,\n-\t\t\t\t\t\t       file_name))\n+\telse if (ctx->to_include &&\n+\t\t !string_list_has_string(ctx->to_include, file_name))\n \t\treturn 0;\n \treturn 1;\n }\n@@ -121,7 +135,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,\n \tif (ends_with(file_name, \".idx\")) {\n \t\tdisplay_progress(ctx->progress, ++ctx->pack_paths_checked);\n \n-\t\tif (!should_include_pack(ctx, file_name, 1))\n+\t\tif (!should_include_pack(ctx, file_name))\n \t\t\treturn;\n \n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n@@ -880,9 +894,6 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \tuint32_t i;\n \n \tfor (i = 0; i < ctx->m->num_packs; i++) {\n-\t\tif (!should_include_pack(ctx, ctx->m->pack_names[i], 0))\n-\t\t\tcontinue;\n-\n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \n \t\tif (flags & MIDX_WRITE_REV_INDEX || preferred_pack_name) {\n@@ -937,7 +948,15 @@ static int write_midx_internal(const char *object_dir,\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n \t\t\t  midx_name.buf);\n \n-\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n+\tif (!packs_to_include) {\n+\t\t/*\n+\t\t * Only reference an existing MIDX when not filtering which\n+\t\t * packs to include, since all packs and objects are copied\n+\t\t * blindly from an existing MIDX if one is present.\n+\t\t */\n+\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n+\t}\n+\n \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n \t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n \t\tctx.m = NULL;\n@@ -946,7 +965,6 @@ static int write_midx_internal(const char *object_dir,\n \tctx.nr = 0;\n \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n \tctx.info = NULL;\n-\tctx.to_include = packs_to_include;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n \n \tif (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,\n@@ -963,6 +981,8 @@ static int write_midx_internal(const char *object_dir,\n \telse\n \t\tctx.progress = NULL;\n \n+\tctx.to_include = packs_to_include;\n+\n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex cc7220b6c0..916da389b6 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -551,4 +551,34 @@ do\n \t'\n done\n \n+test_expect_success 'remove one packfile between MIDX bitmap writes' '\n+\tgit init remove-pack-between-writes &&\n+\t(\n+\t\tcd remove-pack-between-writes &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\t# Create packs with the prefix \"pack-A\", \"pack-B\",\n+\t\t# \"pack-C\" to impose a lexicographic order on these\n+\t\t# packs so the pack being removed is always from the\n+\t\t# middle.\n+\t\tpackdir=.git/objects/pack &&\n+\t\tA=\"$(echo A | git pack-objects $packdir/pack-A --revs)\" &&\n+\t\tB=\"$(echo B | git pack-objects $packdir/pack-B --revs)\" &&\n+\t\tC=\"$(echo C | git pack-objects $packdir/pack-C --revs)\" &&\n+\n+\t\tgit multi-pack-index write --bitmap &&\n+\n+\t\tcat >in <<-EOF &&\n+\t\tpack-A-$A.idx\n+\t\tpack-C-$C.idx\n+\t\tEOF\n+\t\tgit multi-pack-index write --bitmap --stdin-packs <in &&\n+\n+\t\tgit rev-list --test-bitmap HEAD\n+\t)\n+'\n+\n test_done\n-- \n2.45.2.448.g06de4005f1\n\n"},{"id":"496932","messageId":"4b006f44a53fb8e0a27a89b78e43a5ef225cebb8.1718126886.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"cover.1718126886.git.me@ttaylorr.com","subject":"[PATCH v2 2/3] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-11T17:28:20Z","receivedAt":"2024-06-11T17:28:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When performing multi-pack reuse, reuse_partial_packfile_from_bitmap()\nis responsible for generating an array of bitmapped_pack structs from\nwhich to perform reuse.\n\nIn the multi-pack case, we loop over the MIDXs packs and copy the result\nof calling `nth_bitmapped_pack()` to construct the list of reusable\npaths.\n\nBut we may also want to do pack-reuse over a single pack, either because\nwe only had one pack to perform reuse over (in the case of single-pack\nbitmaps), or because we explicitly asked to do single pack reuse even\nwith a MIDX[^1].\n\nWhen this is the case, the array we generate of reusable packs contains\nonly a single element, which is either (a) the pack attached to the\nsingle-pack bitmap, or (b) the MIDX's preferred pack.\n\nIn 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,\n2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()\nfunction and stopped assigning the pack_int_id field when reusing only\nthe MIDX's preferred pack. This results in an uninitialized read down in\ntry_partial_reuse() like so:\n\n    ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value\n    #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8\n    #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8\n    #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3\n    #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3\n    #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\n    #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\n    #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\n\nwhich happens when try_partial_reuse() tries to call\nmidx_pair_to_pack_pos() when it tries to reject cross-pack deltas.\n\nAvoid the uninitialized read by ensuring that the pack_int_id field is\nset in the single-pack reuse case by setting it to either the MIDX\npreferred pack's pack_int_id, or '-1', in the case of single-pack\nbitmaps.  In the latter case, we never read the pack_int_id field, so\nthe choice of '-1' is intentional as a \"garbage in, garbage out\"\nmeasure.\n\nGuard against further regressions in this area by adding a test which\nensures that we do not throw out deltas from the preferred pack as\n\"cross-pack\" due to an uninitialized pack_int_id.\n\n[^1]: This can happen for a couple of reasons, either because the\n  repository is configured with 'pack.allowPackReuse=(true|single)', or\n  because the MIDX was generated prior to the introduction of the BTMP\n  chunk, which contains information necessary to perform multi-pack\n  reuse.\n\nReported-by: Kyle Lippincott <spectral@google.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c               | 13 +++++++++++++\n t/t5332-multi-pack-reuse.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fe8e8a51d3..1d6b7f2826 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2073,6 +2073,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n \t} else {\n \t\tstruct packed_git *pack;\n+\t\tuint32_t pack_int_id;\n \n \t\tif (bitmap_is_midx(bitmap_git)) {\n \t\t\tuint32_t preferred_pack_pos;\n@@ -2083,12 +2084,24 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\t}\n \n \t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n+\t\t\tpack_int_id = preferred_pack_pos;\n \t\t} else {\n \t\t\tpack = bitmap_git->pack;\n+\t\t\t/*\n+\t\t\t * Any value for 'pack_int_id' will do here. When we\n+\t\t\t * process the pack via try_partial_reuse(), we won't\n+\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n+\t\t\t * bitmap.\n+\t\t\t *\n+\t\t\t * Use '-1' as a sentinel value to make it clear\n+\t\t\t * that we do not expect to read this field.\n+\t\t\t */\n+\t\t\tpack_int_id = -1;\n \t\t}\n \n \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n \t\tpacks[packs_nr].p = pack;\n+\t\tpacks[packs_nr].pack_int_id = pack_int_id;\n \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n \t\tpacks[packs_nr].bitmap_pos = 0;\n \ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 3c20738bce..ed823f37bc 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -204,4 +204,30 @@ test_expect_success 'omit delta from uninteresting base (cross pack)' '\n \ttest_pack_objects_reused_all $(($objects_nr - 1)) $packs_nr\n '\n \n+test_expect_success 'non-omitted delta in MIDX preferred pack' '\n+\ttest_config pack.allowPackReuse single &&\n+\n+\tcat >p1.objects <<-EOF &&\n+\t$(git rev-parse $base)\n+\t^$(git rev-parse $delta^)\n+\tEOF\n+\tcat >p2.objects <<-EOF &&\n+\t$(git rev-parse F)\n+\tEOF\n+\n+\tp1=\"$(git pack-objects --revs $packdir/pack <p1.objects)\" &&\n+\tp2=\"$(git pack-objects --revs $packdir/pack <p2.objects)\" &&\n+\n+\tcat >in <<-EOF &&\n+\tpack-$p1.idx\n+\tpack-$p2.idx\n+\tEOF\n+\tgit multi-pack-index write --bitmap --stdin-packs \\\n+\t\t--preferred-pack=pack-$p1.pack <in &&\n+\n+\tgit show-index <$packdir/pack-$p1.idx >expect &&\n+\n+\ttest_pack_objects_reused_all $(wc -l <expect) 1\n+'\n+\n test_done\n-- \n2.45.2.448.g06de4005f1\n\n"},{"id":"496933","messageId":"06de4005f15a970628cef1aeb7f159ca05b8e34d.1718126886.git.me@ttaylorr.com","threadId":"61610","inReplyTo":"cover.1718126886.git.me@ttaylorr.com","subject":"[PATCH v2 3/3] pack-revindex.c: guard against out-of-bounds pack lookups","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-11T17:28:24Z","receivedAt":"2024-06-11T17:28:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function midx_key_to_pack_pos() is a helper function used by\nmidx_to_pack_pos() and midx_pair_to_pack_pos() to translate a (pack,\noffset) tuple into a position into the MIDX pseudo-pack order.\n\nEnsure that the pack ID given to midx_pair_to_pack_pos() is bounded by\nthe number of packs within the MIDX to prevent, for instance,\nuninitialized memory from being used as a pack ID.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-revindex.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex fc63aa76a2..93ffca7731 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -527,6 +527,9 @@ static int midx_key_to_pack_pos(struct multi_pack_index *m,\n {\n \tuint32_t *found;\n \n+\tif (key->pack >= m->num_packs)\n+\t\tBUG(\"MIDX pack lookup out of bounds (%\"PRIu32\" >= %\"PRIu32\")\",\n+\t\t    key->pack, m->num_packs);\n \t/*\n \t * The preferred pack sorts first, so determine its identifier by\n \t * looking at the first object in pseudo-pack order.\n-- \n2.45.2.448.g06de4005f1\n"},{"id":"496935","messageId":"ZmiJ6H6xkvc5WnC9@nand.local","threadId":"61610","inReplyTo":"cover.1718126886.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/3] midx: various brown paper bag fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-11T17:31:20Z","receivedAt":"2024-06-11T17:31:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 11, 2024 at 01:28:07PM -0400, Taylor Blau wrote:\n> This series fixes a couple of brown paper bag bugs in the MIDX/bitmap\n> code.\n>\n> This version is basically identical to the previous round, except that\n> we use the sentinel value \"-1\" as the 'pack_int_id' when reusing from a\n> single (non-MIDX'd) pack. This is a \"garbage in, garbage out\" measure to\n> ensure that we notice any regressions here loudly.\n>\n> Thanks in advance for your review!\n\nI must have forgotten to tag my original \"v2\" as such, since my scripts\ndecided that this is actually v2. Apologies for any confusion, this\nlatest version is certainly v3, the range-diff looks like so:\n\n--- 8< ---\n\n1:  c62daacd1d = 1:  0bdf925366 midx-write.c: do not read existing MIDX with `packs_to_include`\n2:  9b78fa2923 ! 2:  4b006f44a5 pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse\n    @@ Commit message\n\n         Avoid the uninitialized read by ensuring that the pack_int_id field is\n         set in the single-pack reuse case by setting it to either the MIDX\n    -    preferred pack's pack_int_id, or '0', in the case of single-pack\n    +    preferred pack's pack_int_id, or '-1', in the case of single-pack\n         bitmaps.  In the latter case, we never read the pack_int_id field, so\n    -    the choice of '0' is arbitrary.\n    +    the choice of '-1' is intentional as a \"garbage in, garbage out\"\n    +    measure.\n\n         Guard against further regressions in this area by adding a test which\n         ensures that we do not throw out deltas from the preferred pack as\n    @@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitm\n     +\t\t\t * process the pack via try_partial_reuse(), we won't\n     +\t\t\t * use the `pack_int_id` field since we have a non-MIDX\n     +\t\t\t * bitmap.\n    ++\t\t\t *\n    ++\t\t\t * Use '-1' as a sentinel value to make it clear\n    ++\t\t\t * that we do not expect to read this field.\n     +\t\t\t */\n    -+\t\t\tpack_int_id = 0;\n    ++\t\t\tpack_int_id = -1;\n      \t\t}\n\n      \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n3:  e08f679dec = 3:  06de4005f1 pack-revindex.c: guard against out-of-bounds pack lookups\n\n--- >8 ---\n\nThanks,\nTaylor\n"}]}