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

[Martin Storsjö]

On Fri, 8 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.
>
> Thinking about this, I don't think a comment _here_ is a good idea.
> I'd be more inclined to add it to the DECLARE_ALIGNED macro.
> People defining a new aligned member variable are at least a little more 
> likely to read that, compared to something next to this random define 
> that's not even in a header.

I still think it'd be good to have a comment here, too, to explain this 
code to whoever is looking at it (why 32 or 64, why not 16/32/64 etc). I 
agree that it can be good to mention it near DECLARE_ALIGNED as well, but 
these comments serve different readers.

// Martin