On Sun, Jun 09, 2024 at 04:00:57PM -0400, Taylor Blau wrote:
> A more likely outcome would be if a Git client who was using MIDX
> bitmaps, but only doing single-pack reuse tried to generate a pack to
> push to some remote (or vice-versa, replacing client and server/remote)
> which ended up being corrupt in some fashion. I haven't had enough of a
> chance to determine if this possible, though I suspect it is.
OK. I looked a little more, and I think that though this is possible in
theory, it is exceedingly rare to hit in practice.
The thing that we care about reading pack_int_id is for rejecting
cross-pack deltas in try_partial_reuse(). A cross-pack delta occurs, for
some delta/base pair when either half of the pair was selected from
different packs in the MIDX. When this is the case, we can't verbatim
reuse the delta object, since the base object may be selected from a
pack that was reused later in the MIDX, thus we'd have to convert to a
REF_DELTA on the fly.
(This is a bit of an aside, but it is technically possible to allow
cross-pack deltas if you are careful, I just didn't implement it thus
far, though I would like to do so in the future).
So when we try and do a partial reuse (i.e. on a single object instead
of saying, "these N words in the bitmap of objects that we want to pack
are all ones, so we can blindly reuse a chunk of objects from the
beginning of this pack) we check if the object we're reusing is either
an OFS_DELTA or a REF_DELTA. If it is, *and* we're reusing it from a
MIDX, we need to check that its base object was selected in the same
pack.
That's what we're doing when we call:
midx_pair_to_pack_pos(bitmap_git->midx, pack->pack_int_id,
base_offset, &base_bitmap_pos);
If we can't find the base object in the same pack, we toss out the
cross-pack delta and kick it back to the regular pack generation logic.
So in order to hit this in a way that would produce a broken pack, you'd
have to have the uninitialized memory at pack->pack_int_id be set to the
same value as some MIDX'd pack's pack_int_id, *and* have that pack
happen to contain the same object as the base of some delta that you
would otherwise want to throw out.
If that were all to happen, then you would likely end up with a broken
pack. But I think in the vast majority of cases try_partial_reuse()
would read garbage out of pack->pack_int_id, and kick it back to the
regular pack generation logic, and produce a correct pack (albeit doing
so using a slightly slower path).
But this is all much too close for comfort for me. I think that this
points to a gap in our test coverage, and that we should ensure that
this case is covered.
(Note, I'm not sure exactly how to do this, since such a test would
depend on an uninitialized read. I'll think about this a bit more and
see if it is possible to write such a test.)
But in summary, though I think it is possible for either a client to
send a broken push to a server (if the client were using MIDX bitmaps
and only doing single-pack reuse) or for a server to send a broken pack
to a client (if the server was also using MIDX bitmaps in the same
fashion), I think that it is exceedingly rare to hit in practice. Which
is not the same as saying that it is impossible, of course, but I am
confident with the fix I posted in:
https://lore.kernel.org/git/4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@xxxxxxxxxxxx/T/#u
to fix the issue. Note also that I don't think a repository would be
able to actually corrupt itself using this bug, since we don't bother
taking this code path at all during repacks.
So in short, I think the fix I posted above should be tracked down to
'maint' at least for the 2.45.x series. It will avoid the MSan failures
and more importantly the issue I described above. I would also like to
find a way to further test this case so that we aren't bit by such a bug
in the future.
Thanks,
Taylor
