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

"Pablo Sabater" <pabloosabaterr@gmail.com> writes:

"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 <stdalign.h>, 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.