On Sun, Jun 09, 2024 at 11:27:35AM -0400, Taylor Blau wrote:
> In 795006fff4 (pack-bitmap: gracefully handle missing BTMP chunks,
> 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap()
> function and stopped assigning the pack_int_id field when reusing only
> the MIDX's preferred pack. This results in an uninitialized read down in
> try_partial_reuse() like so:
I feel like I'm blind, but I cannot see how the patch changed what we do
with `pack_int_id`. It's not mentioned a single time in the diff, so how
did it have the effect of not setting it anymore?
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index fe8e8a51d3..8b9a2c698f 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -2073,6 +2073,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
> QSORT(packs, packs_nr, bitmapped_pack_cmp);
> } else {
> struct packed_git *pack;
> + uint32_t pack_int_id;
>
> if (bitmap_is_midx(bitmap_git)) {
> uint32_t preferred_pack_pos;
> @@ -2083,12 +2084,21 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
> }
>
> pack = bitmap_git->midx->packs[preferred_pack_pos];
> + pack_int_id = preferred_pack_pos;
> } else {
> pack = bitmap_git->pack;
> + /*
> + * Any value for 'pack_int_id' will do here. When we
> + * process the pack via try_partial_reuse(), we won't
> + * use the `pack_int_id` field since we have a non-MIDX
> + * bitmap.
> + */
> + pack_int_id = 0;
> }
>
> ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
> packs[packs_nr].p = pack;
> + packs[packs_nr].pack_int_id = pack_int_id;
> packs[packs_nr].bitmap_nr = pack->num_objects;
> packs[packs_nr].bitmap_pos = 0;
Okay, the patch looks trivial enough. I was wondering whether we might
want to `memset(&packs[packs_nr], 0, sizeof(packs[packs_nr]))` as it
feels rather easy to miss initialization of one of the members. But on
the other hand, this might also cause us to paper over bugs if we did
that.
Patrick
Attachment:
signature.asc
Description: PGP signature
