Subject: Re: [PATCH 0/2] midx: prevent bitmap corruption when permuting pack order

From: Taylor Blau <[email-masked]>
Date: Fri, 10 Dec 2021 17:31:27 -0500

On Fri, Dec 10, 2021 at 01:36:02PM -0500, Taylor Blau wrote:
> On Wed, Dec 08, 2021 at 02:55:13PM -0500, Jeff King wrote:
> > On Wed, Dec 08, 2021 at 02:30:17PM -0500, Derrick Stolee wrote:
> >
> > > > Taylor Blau (2):
> > > >   t5326: demonstrate bitmap corruption after permutation
> > > >   midx.c: make changing the preferred pack safe
> > >
> > > Just chiming in to say that I reviewed an earlier version of this series
> > > and the version in this submission looks good to me.
> >
> > Ditto. ;)
>
> All three of us missed that this PORD chunk actually contains the
> psuedo-pack position for every object in the MIDX. That is OK, but it's
> definitely adding more than 4 bytes per pack to the MIDX (in practice,
> it's adding 4 bytes per object).
>
> I'm semi-OK with this direction, since it's tantamount to storing the
> .rev file's contents in the MIDX itself. And even though we're not
> reading from it, it is doing the thing we need it to which is causing
> the MIDX to change its checksum when the object order changes.
>
> But I'm curious what both of your thoughts are before moving forward.

To just add a little bit more detail before I mostly close my computer
for the weekend:

The key part of this bug is that the MIDX checksum could remain
unchanged even when the object order used to write the MIDX bitmap does,
and that's what the first patch here is demonstrating.

Having PORD contain the same data as the .rev file still accomplishes
our original goal of preventing this bug, because it forces the checksum
to change when the object order does. But it's definitely more invasive
than I had imagined.

I had originally imagined that storing the preferred pack's identity
alone would be enough to solve this bug. But that isn't quite so,
because we break ties among duplicate objects first by prefered-ness,
then by their pack's mtime. So that could change too, and it would cause
us to break in the same way.

At the bare minimum you need an ordering of all of the packs in the
MIDX (like I had originally imagined here). At most, we could do
something like what is unintentionally written here, which would allow
us to get rid of MIDX .rev files entirely. I think doing the former is
simpler, and I am not sure if there are practical advantages to the
latter.

But I'm definitely curious as to what others think would be a good
direction to pursue.

Thanks,
Taylor