Timo Rothenpieler <t...@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-requ...@ffmpeg.org with subject "unsubscribe".
