[FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align

[Timo Rothenpieler]

On 11.02.2024 15:22, Rémi Denis-Courmont wrote:
> Le perjantaina 9. helmikuuta 2024, 21.22.17 EET Timo Rothenpieler a écrit :
>> On 13.01.2024 16:46, 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.
>>>
>>> This patch limits the maximum alignment to the maximum possible simd
>>> alignment according to configure.
>>> While not perfect, it at the very least gets rid of a lot of UB, by
>>> matching up the maximum DECLARE_ALIGNED value with the alignment of heap
>>> allocations done by lavu.
>>> ---
>>>
>>>    libavutil/mem.c          |  8 +++++++-
>>>    libavutil/mem_internal.h | 14 ++++++++------
>>>    2 files changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>>> index 36b8940a0c..b5bcaab164 100644
>>> --- a/libavutil/mem.c
>>> +++ b/libavutil/mem.c
>>> @@ -62,7 +62,13 @@ void  free(void *ptr);
>>>
>>>    #endif /* MALLOC_PREFIX */
>>>
>>> -#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))
>>> +#if defined(_MSC_VER)
>>> +/* MSVC does not support conditionally limiting alignment.
>>> +   Set minimum value here to maximum used throughout the codebase. */
>>> +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : 32)
> 
> Not that I care whatsoever, but are we assuming that MSVC supports only x86?
> Otherwise, this conditional definition does not make much sense and seems very
> sketchy. In fact, I don't see the point in making this distinction at all
> (*unlike* below).
> 

MSVC straight up _does not support_ putting conditionals into its 
alignment macros.
It initially had the same treatment, but failed with compile errors.