Re: [PATCH v2 0/2] pseudo-merge: various small fixes

[Elijah Newren <newren@gmail.com>]

On Fri, Jun 14, 2024 at 7:23 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> Here is a small reroll of a couple of patches I wrote to fix various
> small issues with the tb/pseudo-merge-reachability-bitmaps topic.
>
> The only change since last time is replacing:
>
>     if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)
>
> with:
>
>     if (st_add(st_mult(index->pseudo_merges.nr, sizeof(uint64_t)), 24) > table_size)
>
> based on helpful review from Junio. For convenience, a range-diff is
> below. Thanks in advance for any final review on this topic :-).
>
> Taylor Blau (2):
>   Documentation/technical/bitmap-format.txt: add missing position table
>   pack-bitmap.c: ensure pseudo-merge offset reads are bounded
>
>  Documentation/technical/bitmap-format.txt | 9 +++++++++
>  pack-bitmap.c                             | 5 +++++
>  2 files changed, 14 insertions(+)
>
> Range-diff against v1:
> -:  ---------- > 1:  a71ec05e5d Documentation/technical/bitmap-format.txt: add missing position table
> 1:  0a16399d14 ! 2:  8abd564e7c pack-bitmap.c: ensure pseudo-merge offset reads are bounded
>     @@ Commit message
>          end of the mmap'd region.
>
>          Prevent this by ensuring that we have at least `table_size - 24` many
>     -    bytes available to read (subtracting 24 as the length of the metadata
>     -    component).
>     +    bytes available to read (adding 24 to the left-hand side of our
>     +    inequality to account for the length of the metadata component).
>
>          This is sufficient to prevent us from reading off the end of the
>          pseudo-merge extension, and ensures that all of the get_be64() calls
>     @@ pack-bitmap.c: static int load_bitmap_header(struct bitmap_index *index)
>                                 index->pseudo_merges.commits_nr = get_be32(index_end - 20);
>                                 index->pseudo_merges.nr = get_be32(index_end - 24);
>
>     -+                          if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)
>     ++                          if (st_add(st_mult(index->pseudo_merges.nr,
>     ++                                             sizeof(uint64_t)),
>     ++                                     24) > table_size)
>      +                                  return error(_("corrupted bitmap index file, pseudo-merge table too short"));
>      +
>                                 CALLOC_ARRAY(index->pseudo_merges.v,
>
> base-commit: 0b7500dc66ffcb6b1ccc3332715936a59c6b5ce4
> --
> 2.45.0.33.g0a16399d14.dirty

Series looks good to me.