Re: [PATCH 1/4] midx repack: avoid integer overflow on 32 bit systems

["D. Ben Knoble" <ben.knoble@gmail.com>]

On Tue, May 20, 2025 at 11:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> On a 32 bit system "git multi-pack-index --repack --batch-size=120M"
> failed with
>
>     fatal: size_t overflow: 6038786 * 1289
>
> The calculation to estimated size of the objects in the pack referenced
> by the multi-pack-index uses st_mult() to multiply the pack size by the
> number of referenced objects before dividing by the total number of
> objects in the pack. As size_t is 32 bits on 32 bit systems this
> calculation easily overflows. Fix this by using 64bit arithmetic instead.
>
> Also fix a potential overflow when caluculating the total size of the
> objects referenced by the multipack index with a batch size larger
> than SIZE_MAX / 2. In that case
>
>     total_size += estimated_size
>
> can overflow as both total_size and estimated_size can be greater that
> SIZE_MAX / 2. This is addressed by using saturating arithmetic for the
> addition.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  git-compat-util.h | 16 ++++++++++++++++
>  midx-write.c      | 12 ++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 36b9577c8d4..4678e21c4cb 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -668,6 +668,22 @@ static inline int cast_size_t_to_int(size_t a)
>         return (int)a;
>  }
>
> +static inline uint64_t u64_mult(uint64_t a, uint64_t b)
> +{
> +       if (unsigned_mult_overflows(a, b))
> +               die("uint64_t overflow: %"PRIuMAX" * %"PRIuMAX,
> +                   (uintmax_t)a, (uintmax_t)b);
> +       return a * b;
> +}
> +
> +static inline uint64_t u64_add(uint64_t a, uint64_t b)
> +{
> +       if (unsigned_add_overflows(a, b))
> +               die("uint64_t overflow: %"PRIuMAX" + %"PRIuMAX,
> +                   (uintmax_t)a, (uintmax_t)b);
> +       return a + b;
> +}
> +
>  /*
>   * Limit size of IO chunks, because huge chunks only cause pain.  OS X
>   * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
> diff --git a/midx-write.c b/midx-write.c
> index dd3b3070e55..c7cb2315431 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1699,19 +1699,23 @@ static void fill_included_packs_batch(struct repository *r,
>         for (i = 0; total_size < batch_size && i < m->num_packs; i++) {
>                 int pack_int_id = pack_info[i].pack_int_id;
>                 struct packed_git *p = m->packs[pack_int_id];
> -               size_t expected_size;
> +               uint64_t expected_size;
>
>                 if (!want_included_pack(r, m, pack_kept_objects, pack_int_id))
>                         continue;
>
> -               expected_size = st_mult(p->pack_size,
> -                                       pack_info[i].referenced_objects);
> +               expected_size = uint64_mult(p->pack_size,
> +                                           pack_info[i].referenced_objects);
>                 expected_size /= p->num_objects;
>
>                 if (expected_size >= batch_size)
>                         continue;
>
> -               total_size += expected_size;
> +               if (unsigned_add_overflows (total_size, (size_t)expected_size))

Style nit (only in case Taylor's approach doesn't prove better): I
wasn't expecting a space between the function and its argument list.

> +                       total_size = SIZE_MAX;
> +               else
> +                       total_size += expected_size;
> +
>                 include_pack[pack_int_id] = 1;
>         }
>
> --
> 2.49.0.897.gfad3eb7d210
>
>


-- 
D. Ben Knoble