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

From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Oct 27, 2026, 11:52 UTC
Message-ID: <DMFLKY90144W.2W4T9I9V2PEYO@gmail.com>
In-Reply-To: <pull.2276.v7.git.1793098852493.gitgitgadget@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]

Show 9 quoted lines

>
> 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]

Show 13 quoted lines

>
> 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.

Show 6 quoted lines

>           - 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).

Show 14 quoted lines

>       
>      -    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