Re: MSan failures in pack-bitmap

From: Jeff King <peff@peff.net>
Date: Jun 11, 2024, 08:02 UTC
Message-ID: <20240611080220.GG3248245@coredump.intra.peff.net>
In-Reply-To: <ZmYPjuGnh+UvT5dt@nand.local>

On Sun, Jun 09, 2024 at 04:24:46PM -0400, Taylor Blau wrote:

Show 13 quoted lines

> 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@ttaylorr.com/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.

Right, I think this is mostly just a vanilla bug. It could cause bogus packs on fetch, but the clients would realize it. If people aren't screaming about broken fetches, then it is not happening very often (and your analysis nicely explains why). But I don't see how it could ever cause corruption.

Directing the fix to "maint" as you suggest is sensible, but I don't think it merits any special treatment.

Show 5 quoted lines

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

I don't think we can test the case where the bug would produce a bogus pack, since that implies guessing the uninitialized data. I guess we could come up with a case where try_partial_reuse() should say "this is OK to reuse", but the bogus pack_int_id prevents it. Which implies looking at the resulting pack and checking that some delta is there that we expect?

-Peff