Re: [PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 10, 2024, 05:55 UTC
Message-ID: <ZmaVYnmgyAr0vapK@tanuki>
In-Reply-To: <4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com>

On Sun, Jun 09, 2024 at 11:27:35AM -0400, Taylor Blau wrote:

Show 5 quoted lines

> 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?

Show 33 quoted lines

> 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