Re: MSan failures in pack-bitmap

[Taylor Blau <me@ttaylorr.com>]

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

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