[FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes

[Martin Storsjö]

On Wed, 6 Dec 2023, Timo Rothenpieler wrote:

>
>
> On 06/12/2023 14:25, Martin Storsjö wrote:
>> On Sun, 3 Dec 2023, Timo Rothenpieler wrote:
>> 
>>> FFmpeg has instances of DECLARE_ALIGNED(32, ...) in a lot of structs,
>>> which then end up heap-allocated.
>>> By declaring any variable in a struct, or tree of structs, to be 32 byte
>>> aligned, it allows the compiler to safely assume the entire struct
>>> itself is also 32 byte aligned.
>>>
>>> This might make the compiler emit code which straight up crashes or
>>> misbehaves in other ways, and at least in one instances is now
>>> documented to actually do (see ticket 10549 on trac).
>>> The issue there is that an unrelated variable in SingleChannelElement is
>>> declared to have an alignment of 32 bytes. So if the compiler does a copy
>>> in decode_cpe() with avx instructions, but ffmpeg is built with
>>> --disable-avx, this results in a crash, since the memory is only 16 byte
>>> aligned.
>>> Mind you, even if the compiler does not emit avx instructions, the code
>>> is still invalid and could misbehave. It just happens not to. Declaring
>>> any variable in a struct with a 32 byte alignment promises 32 byte
>>> alignment of the whole struct to the compiler.
>>>
>>> Instead of now going through all instances of variables in structs
>>> being declared as 32 byte aligned, this patch bumps the minimum alignment
>>> to 32 bytes.
>>> ---
>>> libavutil/mem.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>>> index 36b8940a0c..26a9b9753b 100644
>>> --- a/libavutil/mem.c
>>> +++ b/libavutil/mem.c
>>> @@ -62,7 +62,7 @@ void  free(void *ptr);
>>>
>>> #endif /* MALLOC_PREFIX */
>>>
>>> -#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))
>>> +#define ALIGN (HAVE_AVX512 ? 64 : 32)
>> 
>> LGTM
>> 
>> It could be good to add a comment here, to indicate how this value 
>> relates to the alignemnts used in structs.
>> 
>> For others who commented in this thread, it all boils down to something 
>> like this:
>> 
>> struct MyData {
>>      uint8_t __attribute__((aligned(32))) aligned_data[1024];
>> };
>
> It's even a bit more complex than that.
> The case that's crashing right now is a member that has no alignment 
> declared on itself at all.
> But another member of the same struct does, and so the compiler assumes 
> the whole struct to be aligned.

Ah, tricky! Yeah, that's also a valid assumption for the compiler, but 
also a rather non-obvious one.

// Martin