On Sat, Jun 08, 2024 at 04:18:55AM -0400, Jeff King wrote:

On Fri, Jun 07, 2024 at 07:43:56PM -0700, Kyle Lippincott wrote:

quoted

I believe what's happening is that pack-bitmap.c:2091 grows the packs
list and sets up some of the fields, but doesn't set pack_int_id. We
then use it at pack-bitmap.c:1888.

I investigated, but couldn't prove to myself what value should be
placed there while growing it, or if it's incorrect to read from it in
this case (so we shouldn't be in pack-bitmap.c:1888 with this pack).

Hmm, I'm not sure.

In reuse_partial_packfile_from_bitmap(), the code path that creates the
struct only kicks in when the "multi_pack_reuse" flag isn't set. Which
generally would correspond to whether we have a midx. And then the code
in try_partial_reuse() that uses the struct similarly checks
bitmap_is_midx() before looking at the pack_int_id field.

But that changed in 795006fff4 (pack-bitmap: gracefully handle missing
BTMP chunks, 2024-04-15), where we also disable multi_pack_reuse if we
have a midx but it has no BTMP chunk. So we end up in the non-multi code
path to create the struct, but then try_partial_reuse() still realizes
we have a midx and uses that code path.

I guess this gets into the "we have a midx, but are only doing reuse out
of a single pack" case. Which I think is supported, but I'm not familiar
enough with the code to know where the assumption is going wrong.

That's right. We support single-pack reuse even with a MIDX either (a)
because it was configured that way with
pack.allowPackReuse=(true|single), or (b) because the MIDX has no BTMP
chunk, which is what prompted the change in 795006fff4.

When in that case, our reuse packfile is either the pack attached to a
single-pack bitmap, or the MIDX's preferred pack. When using the MIDX's
preferred pack, we need to make sure that we correctly assign the
pack_int_id to be the ID of the preferred pack. (We use this field to
reject cross-pack deltas later on in try_partial_reuse(), which is where
the MSan failure is happening).

The fix should be to set pack_int_id to the preferred pack's ID in the
MIDX case, and an arbitrary value in the single-pack bitmap case. I
posted a patch which should fix that here:

    https://lore.kernel.org/git/4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com/T/#u (local)

Unfortunately, the regression happened in 795006fff4, so this is in a
released version of Git. But this is all behind a configuration option,
so affected users can reasonably work around this issue.

Thanks,
Taylor