From: Srujith Poojary Date: Tue, 27 Oct 2026 16:57:46 GMT Subject: Re: [PATCH v7] diff-delta: use safe integer arithmetic Message-ID: In-Reply-To: On Tue, Oct 27, 2026 at 9:57 PM Junio C Hamano wrote: > > "Pablo Sabater" writes: > > > 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 > > "exactly 16 bytes"? That sounds fishy. > > It is implementation defined, a naive expectation is it would be > more common to make it occupy 12 bytes, and we should expect it to > be larger like 16 bytes depending on platforms. > > The C standard (e.g., ISO/IEC 9899:1999) has this: > > Within a structure object, the non-bit-field members and the > units in which bit-fields reside have addresses that increase in > the order in which they are declared. A pointer to a structure > object, suitably converted, points to its initial member (or if > that member is a bit-field, then to the unit in which it > resides), and vice versa. There may be unnamed padding within a > structure object, but not at its beginning. > > For a 32-bit platform, the minimum natural alignment for a 4-byte > unsigned int, unsigned char *, and a pointer to a struct, are all 4 > bytes. However, the standard specifies minimum requirements, not > maximums. A compiler is entirely free to choose a stricter > alignment (such as 16 bytes) for performance optimization reasons, > e.g., to align for better cache line utilization or vectorization. > > I think your math is correct in that the computation for allocation > size would not overflow if we can assume the struct involved is > always 12-byte on 32-bit systems. That would make this patch > unnecessary (and the patch is unnecessary on 64-bit systems with > wider size_t to begin with). But we cannot assume the best case. > > We could assume sizeof(struct unpacked_index_entry) == 12 on > platforms where we can depend on GCC/Clang's __attribute__((packed)) > and C11's , but we are not doing that (and I am not sure > if we want to, as these can introduce different portability issues). > > Thanks for pointing out the fishy description. > > Hi Junio and Pablo, Thank you both for the detailed feedback and guidance. Junio, your explanation regarding implementation-defined struct alignment and why hardcoding exact padding sizes can be misleading across different compilers and platforms was extremely helpful. Pablo, thank you for pushing for precision on the exact bounds and math. I have just submitted v9, which updates the commit message to incorporate these insights: - Shifted the focus to robust defensive programming, acknowledging that struct layout and padding can vary across platforms while using st_mult() and st_add() to protect against any future size variations or wrap-arounds. - Explicitly documented the exact bounds: entries capped by 0xfffffffeU / RABIN_WINDOW, and hsize capped at 2^26 (with a minimum of 16). Thank you both for taking the time to review this thoroughly and help me get it right. Best regards, Srujith