From: Pablo Sabater Date: Tue, 27 Oct 2026 11:52:00 GMT Subject: Re: [PATCH v7] diff-delta: use safe integer arithmetic Message-ID: In-Reply-To: On Tue Oct 27, 2026 at 11:00 AM WET, Srujith A Poojary via GitGitGadget wrote: > From: Srujith A Poojary > [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 I recommend reading the review on v6. [snip] -Pablo