From: Srujith Poojary <poojarysrujith@gmail.com>
Date: Sat, 24 Oct 2026 17:26:08 GMT
Subject: Re: [PATCH v3] diff-delta: use safe integer arithmetic
Message-ID: <CABh_KWhDcPM6rJROmVd7_oGZ7i3tmX5da-DZX=KJmvSyCxNH_A@mail.gmail.com>
In-Reply-To: <xmqqfqxv13d6.fsf@gitster.g>

On Sat, Oct 24, 2026 at 9:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> > Replace manual multiplication and addition in create_delta_index()
> > with safe integer helpers (st_mult, st_add, st_add3) to prevent potential
> > overflows when computing memory allocation sizes.
> >
> > On 32-bit platforms, struct alignment padding could cause the total
> > allocation size calculation to exceed 2^32, making safe integer arithmetic
> > necessary.
>
> I wonder if "X could cause Y" above skips a bit too many steps.  I
> was expecting more along the lines of ...
>
>     We will need to allocate N instances of A and M instances of B, N
>     and M are capped to X and Y respectively by this code
>
>         ... code snippet ...
>
>     and size of an instance of A is 8 and size of B could be either W or
>     Z.  When size of B is W, then the total allocation would be ...
>
> And that analysis applies only on the first allocation.  I didn't
> even look at the packed side of the allocation size, expecting that
> you would know how to analyze after seeing my example on the other
> allocation.
>
> > Note that hsize + 1 inside st_add3() is safe from overflow
> > because hsize is capped at 2^26 early in the function.
>
> It is misleading to say hsize+1 is inside st_add3(), though.
>
>
> > diff --git a/diff-delta.c b/diff-delta.c
> > index 7cbedeb507..efebce3aca 100644
> > --- a/diff-delta.c
> > +++ b/diff-delta.c
> > @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> >       hmask = hsize - 1;
> >
> >       /* allocate lookup index */
> > -     memsize = sizeof(*hash) * hsize +
> > -               sizeof(*entry) * entries;
> > +     memsize = st_add(st_mult(sizeof(*hash), hsize),
> > +                      st_mult(sizeof(*entry), entries));
> >       mem = malloc(memsize);
> >       if (!mem)
> >               return NULL;
> > @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
> >        * Now create the packed index in array form
> >        * rather than linked lists.
> >        */
> > -     memsize = sizeof(*index)
> > -             + sizeof(*packed_hash) * (hsize+1)
> > -             + sizeof(*packed_entry) * entries;
> > +     memsize = st_add3(sizeof(*index),
> > +                       st_mult(sizeof(*packed_hash), hsize + 1),
> > +                       st_mult(sizeof(*packed_entry), entries));
> >       mem = malloc(memsize);
> >       if (!mem) {
> >               free(hash);
> >
> > base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926

Hi Junio,

Thank you for the detailed correction.

I have expanded the commit message to include the step-by-step
breakdown for both the unpacked and packed index allocations,
detailing the caps on hsize (2^26) and entries (2^28) alongside struct
padding on 32-bit systems. I also corrected the phrasing regarding
(hsize + 1) being passed as an argument to st_add3().

I have updated the patch series accordingly.

Best regards,
Srujith