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

[Sam James <sam@gentoo.org>]

Timo Rothenpieler <timo@rothenpieler.org> writes:

> 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)
>> +#else
>> +#define ALIGN (HAVE_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16))
>> +#endif
>>     /* NOTE: if you want to override these functions with your own
>>    * implementations (not recommended) you have to link libav* as
>> diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h
>> index 2448c606f1..e2911b5610 100644
>> --- a/libavutil/mem_internal.h
>> +++ b/libavutil/mem_internal.h
>> @@ -75,18 +75,20 @@
>>    * @param v Name of the variable
>>    */
>>   +#define MAX_ALIGNMENT (HAVE_SIMD_ALIGN_64 ? 64 :
>> (HAVE_SIMD_ALIGN_32 ? 32 : 16))
>> +
>>   #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C)
>> -    #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
>> -    #define DECLARE_ASM_ALIGNED(n,t,v)  t __attribute__ ((aligned (n))) v
>> -    #define DECLARE_ASM_CONST(n,t,v)    const t __attribute__ ((aligned (n))) v
>> +    #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v
>> +    #define DECLARE_ASM_ALIGNED(n,t,v)  t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v
>> +    #define DECLARE_ASM_CONST(n,t,v)    const t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v
>>   #elif defined(__DJGPP__)
>>       #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (FFMIN(n, 16)))) v
>>       #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v
>>       #define DECLARE_ASM_CONST(n,t,v)    static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v
>>   #elif defined(__GNUC__) || defined(__clang__)
>> -    #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
>> -    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__ ((aligned (n))) v
>> -    #define DECLARE_ASM_CONST(n,t,v)    static const t av_used __attribute__ ((aligned (n))) v
>> +    #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v
>> +    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v
>> +    #define DECLARE_ASM_CONST(n,t,v)    static const t av_used __attribute__ ((aligned (FFMIN(n, MAX_ALIGNMENT)))) v
>>   #elif defined(_MSC_VER)
>>       #define DECLARE_ALIGNED(n,t,v)      __declspec(align(n)) t v
>>       #define DECLARE_ASM_ALIGNED(n,t,v)  __declspec(align(n)) t v
>
> ping
>
> We really should fix this before 7.0 (and probably also backport it,
> since UB is UB).
>
> I'm fine with whatever approach, as long as the UB is gone.

Yes please, we keep getting users hitting this.

(There's a packaging improvement we can make which Timo has suggested
and I need to implement, but the issue is there nonetheless.)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".