Re: [PATCH v7] diff-delta: use safe integer arithmetic

>
> On Tue Oct 27, 2026 at 11:00 AM WET, Srujith A Poojary via GitGitGadget wrote:
> > From: Srujith A Poojary <poojarysrujith@gmail.com>
> >
> [snip]
> >
> > For the unpacked index allocation:
> > - sizeof(struct unpacked_index_entry) is exactly 16 bytes on 32-bit systems
> >   (4 bytes for ptr, 4 bytes for val, 4 bytes for inner entry, and 4 bytes
> >   for the next pointer).
> > - sizeof(struct index_entry *) is 4 bytes.
> > - entries is capped by (0xffffffu / RABIN_WINDOW), limiting entries.
> > - hsize is determined as the smallest power of 2 >= (entries / 4), capped
> >   appropriately.
>
> Hi,
>
> I couldn't help but notice that my review was completely ignored. Instead
> of fixing the points I raised, v7 just removes the information, and in
> your reply you say 16 bytes when I clearly wrote it couldn't be 16. It
> feels like I'm talking to something automated, and that's a bit
> frustrating.
>
> If I'm the one who got it wrong and that's why you didn't apply it, I'd
> appreciate it if you told me.
>
> [snip]
> >
> > Range-diff vs v6:
> >
> >  1:  b91a1ed54b ! 1:  8eb6d31788 diff-delta: use safe integer arithmetic
> >      @@ Commit message
> >           helpers (st_mult, st_add, st_add3).
> >
> >           For the unpacked index allocation:
> >      -    - sizeof(struct unpacked_index_entry) is 12 bytes on 32-bit systems,
> >      -      which may be padded to 16 bytes depending on compiler alignment rules.
> >      +    - sizeof(struct unpacked_index_entry) is exactly 16 bytes on 32-bit systems
> >      +      (4 bytes for ptr, 4 bytes for val, 4 bytes for inner entry, and 4 bytes
> >      +      for the next pointer).
>
> sizeof(struct unpacked_index_entry) is 12 bytes on 32-bit, not 16.
> "inner entry" is ptr + val, it's not a separate 4 bytes field.
>
> >           - sizeof(struct index_entry *) is 4 bytes.
> >      -    - entries is capped by (0xffffffu / RABIN_WINDOW) where RABIN_WINDOW is 16,
> >      -      limiting entries to 1,048,575.
> >      -    - hsize is the smallest power of 2 greater than or equal to entries,
> >      -      capped at 1,048,576.
> >      +    - entries is capped by (0xffffffu / RABIN_WINDOW), limiting entries.
>
> entries is capped by 0xfffffffeU / RABIN_WINDOW (l 157), not 0xffffffu.
> Removing the number doesn't fix it.
>
> >      +    - hsize is determined as the smallest power of 2 >= (entries / 4), capped
> >      +      appropriately.
>
> "capped appropriately" doesn't say anything, hsize is capped at 2^26
> (with a minimum of 16).
>
> >
> >      -    While entries is bounded in current code, using st_mult() and st_add()
> >      -    guarantees that any unexpected growth in struct sizes or bounds changes
> >      +    Using st_mult() and st_add() guarantees that any allocation calculations
> >           will safely die() on size_t overflow instead of wrapping around.
> >
> >           For the packed index allocation:
> >           - (hsize + 1) is passed to st_mult(), which is evaluated as an argument
> >      -      inside st_add3(), and is safe from overflow because hsize is capped at
> >      -      1,048,576 early in the function.
> >      +      inside st_add3(), and is safe from overflow because hsize is capped early
> >      +      in the function.
> >
> >           Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
>
> I recommend reading the review on v6.
>
> [snip]
>
> -Pablo
>