Re: [PATCH v2 0/3] midx: various brown paper bag fixes

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 11, 2024, 17:31 UTC
Message-ID: <ZmiJ6H6xkvc5WnC9@nand.local>
In-Reply-To: <cover.1718126886.git.me@ttaylorr.com>

On Tue, Jun 11, 2024 at 01:28:07PM -0400, Taylor Blau wrote:

Show 9 quoted lines

> This series fixes a couple of brown paper bag bugs in the MIDX/bitmap
> code.
>
> This version is basically identical to the previous round, except that
> we use the sentinel value "-1" as the 'pack_int_id' when reusing from a
> single (non-MIDX'd) pack. This is a "garbage in, garbage out" measure to
> ensure that we notice any regressions here loudly.
>
> Thanks in advance for your review!

I must have forgotten to tag my original "v2" as such, since my scripts decided that this is actually v2. Apologies for any confusion, this latest version is certainly v3, the range-diff looks like so:

--- 8< ---

1:  c62daacd1d = 1:  0bdf925366 midx-write.c: do not read existing MIDX with `packs_to_include`
2:  9b78fa2923 ! 2:  4b006f44a5 pack-bitmap.c: avoid uninitialized `pack_int_id` during reuse
    @@ Commit message

         Avoid the uninitialized read by ensuring that the pack_int_id field is
         set in the single-pack reuse case by setting it to either the MIDX
    -    preferred pack's pack_int_id, or '0', in the case of single-pack
    +    preferred pack's pack_int_id, or '-1', in the case of single-pack
         bitmaps.  In the latter case, we never read the pack_int_id field, so
    -    the choice of '0' is arbitrary.
    +    the choice of '-1' is intentional as a "garbage in, garbage out"
    +    measure.

         Guard against further regressions in this area by adding a test which
         ensures that we do not throw out deltas from the preferred pack as
    @@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitm
     +			 * process the pack via try_partial_reuse(), we won't
     +			 * use the `pack_int_id` field since we have a non-MIDX
     +			 * bitmap.
    ++			 *
    ++			 * Use '-1' as a sentinel value to make it clear
    ++			 * that we do not expect to read this field.
     +			 */
    -+			pack_int_id = 0;
    ++			pack_int_id = -1;
      		}

      		ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
3:  e08f679dec = 3:  06de4005f1 pack-revindex.c: guard against out-of-bounds pack lookups

--- >8 ---

Thanks, Taylor