Re: [PATCH] pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse
From: Patrick Steinhardt <hidden>
Date: 2024-06-10 05:55:53
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?
quoted hunk ↗ jump to hunk
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
Attachments
- signature.asc [application/pgp-signature] 833 bytes