| Message ID | a3c28f12020712818060bd50d1483507c9b11556.1718050244.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: various brown paper bag fixes | expand |
On Mon, Jun 10, 2024 at 04:10:53PM -0400, Taylor Blau wrote: > Avoid the uninitialized read by ensuring that the pack_int_id field is > set in the single-pack reuse case by setting it to either the MIDX > preferred pack's pack_int_id, or '0', in the case of single-pack > bitmaps. In the latter case, we never read the pack_int_id field, so > the choice of '0' is arbitrary. Could we set it to some sentinel value for the single-pack case? If we set it to "-1", then the BUG() added in patch 3 would trigger if we did accidentally try to feed it to the midx code. Assuming you do not have 2^32-1 packs, of course. ;) I am OK either way, though. And the rest of the patch looked good. -Peff
Jeff King <peff@peff.net> writes: > On Mon, Jun 10, 2024 at 04:10:53PM -0400, Taylor Blau wrote: > >> Avoid the uninitialized read by ensuring that the pack_int_id field is >> set in the single-pack reuse case by setting it to either the MIDX >> preferred pack's pack_int_id, or '0', in the case of single-pack >> bitmaps. In the latter case, we never read the pack_int_id field, so >> the choice of '0' is arbitrary. > > Could we set it to some sentinel value for the single-pack case? If we > set it to "-1", then the BUG() added in patch 3 would trigger if we did > accidentally try to feed it to the midx code. Assuming you do not have > 2^32-1 packs, of course. ;) Yeah, I had exactly the same reaction.
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; diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh index 3c20738bce..ed823f37bc 100755 --- a/t/t5332-multi-pack-reuse.sh +++ b/t/t5332-multi-pack-reuse.sh @@ -204,4 +204,30 @@ test_expect_success 'omit delta from uninteresting base (cross pack)' ' test_pack_objects_reused_all $(($objects_nr - 1)) $packs_nr ' +test_expect_success 'non-omitted delta in MIDX preferred pack' ' + test_config pack.allowPackReuse single && + + cat >p1.objects <<-EOF && + $(git rev-parse $base) + ^$(git rev-parse $delta^) + EOF + cat >p2.objects <<-EOF && + $(git rev-parse F) + EOF + + p1="$(git pack-objects --revs $packdir/pack <p1.objects)" && + p2="$(git pack-objects --revs $packdir/pack <p2.objects)" && + + cat >in <<-EOF && + pack-$p1.idx + pack-$p2.idx + EOF + git multi-pack-index write --bitmap --stdin-packs \ + --preferred-pack=pack-$p1.pack <in && + + git show-index <$packdir/pack-$p1.idx >expect && + + test_pack_objects_reused_all $(wc -l <expect) 1 +' + test_done
When performing multi-pack reuse, reuse_partial_packfile_from_bitmap() is responsible for generating an array of bitmapped_pack structs from which to perform reuse. In the multi-pack case, we loop over the MIDXs packs and copy the result of calling `nth_bitmapped_pack()` to construct the list of reusable paths. But we may also want to do pack-reuse over a single pack, either because we only had one pack to perform reuse over (in the case of single-pack bitmaps), or because we explicitly asked to do single pack reuse even with a MIDX[^1]. When this is the case, the array we generate of reusable packs contains only a single element, which is either (a) the pack attached to the single-pack bitmap, or (b) the MIDX's preferred pack. 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: ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8 #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8 #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3 #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3 #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27 #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3 #6 0x55c5ccc8fac8 in run_builtin git.c:474:11 which happens when try_partial_reuse() tries to call midx_pair_to_pack_pos() when it tries to reject cross-pack deltas. Avoid the uninitialized read by ensuring that the pack_int_id field is set in the single-pack reuse case by setting it to either the MIDX preferred pack's pack_int_id, or '0', in the case of single-pack bitmaps. In the latter case, we never read the pack_int_id field, so the choice of '0' is arbitrary. Guard against further regressions in this area by adding a test which ensures that we do not throw out deltas from the preferred pack as "cross-pack" due to an uninitialized pack_int_id. [^1]: This can happen for a couple of reasons, either because the repository is configured with 'pack.allowPackReuse=(true|single)', or because the MIDX was generated prior to the introduction of the BTMP chunk, which contains information necessary to perform multi-pack reuse. Reported-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap.c | 10 ++++++++++ t/t5332-multi-pack-reuse.sh | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)