From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 9E1ED49461 for ; Sun, 11 Feb 2024 14:06:19 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 32B6268D142; Sun, 11 Feb 2024 16:06:16 +0200 (EET) Received: from smtp.gentoo.org (woodpecker.gentoo.org [140.211.166.183]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6E87668029D for ; Sun, 11 Feb 2024 16:06:08 +0200 (EET) References: <20240113005716.16018-1-timo@rothenpieler.org> <20240113154600.23366-1-timo@rothenpieler.org> <48f7655d-6311-49df-b813-c0acc9031142@rothenpieler.org> User-agent: mu4e 1.10.8; emacs 30.0.50 From: Sam James To: FFmpeg development discussions and patches Date: Sun, 11 Feb 2024 14:05:25 +0000 Organization: Gentoo In-reply-to: <48f7655d-6311-49df-b813-c0acc9031142@rothenpieler.org> Message-ID: <87mss7p039.fsf@gentoo.org> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Timo Rothenpieler 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".