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

["Pablo Sabater" <pabloosabaterr@gmail.com>]

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