Re: [FFmpeg-devel] [PATCH] avutil/mem: limit alignment to maximum simg align
On 13.01.2024 01:57, 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 | 2 +-
libavutil/mem_internal.h | 20 +++-
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/libavutil/mem.c b/libavutil/mem.c
index 36b8940a0c..62163b4cb3 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_SIMD_ALIGN_64 ? 64 : (HAVE_SIMD_ALIGN_32 ? 32 : 16))
/* 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..ddd3c24806 100644
--- a/libavutil/mem_internal.h
+++ b/libavutil/mem_internal.h
@@ -75,22 +75,24 @@
* @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
-#define DECLARE_ASM_CONST(n,t,v)__declspec(align(n)) static const t v
+#define DECLARE_ALIGNED(n,t,v) __declspec(align(FFMIN(n,
MAX_ALIGNMENT))) t v
+#define DECLARE_ASM_ALIGNED(n,t,v) __declspec(align(FFMIN(n,
MAX_ALIGNMENT))) t v
+#define DECLARE_ASM_CONST(n,t,v)__declspec(align(FFMIN(n,
MAX_ALIGNMENT))) static const t v
Just checked, this does in fact not work with msvc:
libavfilter/af_arnndn.c(122): error C2059: Syntaxfehler: "("
So I guess for MSVC, the alignment will always have to be the full 32 or 64.
#else
#define DECLARE_ALIGNED(n,t,v) t v
#define DECLARE_ASM_ALIGNED(n,t,v) t v
___
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".
[FFmpeg-devel] [PATCH v2] avutil/mem: limit alignment to maximum simd align
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 -- 2.34.1 ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/rtsp: Fix server compatibility issues with rtspclientsink GStreamer plugin
On Fri, Jan 12, 2024 at 10:46:41PM +0200, Paul Orlyk wrote:
> On 1/12/24 21:07, Michael Niedermayer wrote:
> > On Wed, Jan 03, 2024 at 02:51:36PM +0200, Paul Orlyk wrote:
> > > On 12/28/23 21:33, Michael Niedermayer wrote:
> > > > On Wed, Dec 27, 2023 at 03:44:09PM +0200, Paul Orlyk wrote:
> > > > > mode field in Transport header can be sent in upper case so make
> > > > > string
> > > > > comparison case insensitive.
> > > > > Also, GStreamer expects to see mode=record instead of mode=receive in
> > > > > Transport header in response.
> > > > >
> > > > > Signed-off-by: Paul Orlyk
> > > > > ---
> > > > >libavformat/rtsp.c| 4 ++--
> > > > >libavformat/rtspdec.c | 4 ++--
> > > > >2 files changed, 4 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > > > > index 583f5338e8..61e24a5c7a 100644
> > > > > --- a/libavformat/rtsp.c
> > > > > +++ b/libavformat/rtsp.c
> > > > > @@ -1012,8 +1012,8 @@ static void
> > > > > rtsp_parse_transport(AVFormatContext *s,
> > > > >if (*p == '=') {
> > > > >p++;
> > > > >get_word_sep(buf, sizeof(buf), ";, ", &p);
> > > > > -if (!strcmp(buf, "record") ||
> > > > > -!strcmp(buf, "receive"))
> > > > > +if (!av_strcasecmp(buf, "record") ||
> > > > > +!av_strcasecmp(buf, "receive"))
> > > > >th->mode_record = 1;
> > > > >}
> > > > >}
> > > > > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c
> > > > > index 39fd92fb66..d6a223cbc6 100644
> > > > > --- a/libavformat/rtspdec.c
> > > > > +++ b/libavformat/rtspdec.c
> > > > > @@ -303,7 +303,7 @@ static int rtsp_read_setup(AVFormatContext*s,
> > > > > char*
> > > > > host, char *controlurl)
> > > > >rtsp_st->interleaved_min =
> > > > > request.transports[0].interleaved_min;
> > > > patch is damaged by linebreaks
> > > >
> > > >
> > > > [...]
> > >
> > >
> > > Sorry for that. Please find it attached.
> >
> > > rtsp.c|4 ++--
> > > rtspdec.c |4 ++--
> > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > > 8ed5709b8c5cb30aeaa18d609b86b9be0557a06e
> > > 0001-avformat-rtsp-Fix-server-compatibility-issues-with-r.patch
> > > From 156ceeded6cd076b781205adc034144186a9a7ea Mon Sep 17 00:00:00 2001
> > > From: Paul Orlyk
> > > Date: Wed, 27 Dec 2023 15:30:20 +0200
> > > Subject: [PATCH] avformat/rtsp: Fix server compatibility issues with
> > > rtspclientsink GStreamer plugin
> > >
> > > mode field in Transport header can be sent in upper case so make string
> > > comparison case insensitive.
> >
> > > Also, GStreamer expects to see mode=record instead of mode=receive in
> > > Transport header in response.
> >
> > It appears to me that these are 2 seperate issues ?
> > if its 2 patches, i can apply the av_strcasecmp().
>
> Essentially they are. I combined them in the context of GStreamer
> compatibility.
> Would it be better at this point to send them separately?
yes
>
> >
> > For the receive vs record i would prefer to see some quote from a RFC
> > instead
> > of just an implementation
> >
> > thx
> >
> > [...]
>
> Regarding receive vs record:
>
> RFC 7826 "Real-Time Streaming Protocol Version 2.0"
> (https://datatracker.ietf.org/doc/html/rfc7826), section 18.54:
>mode: The mode parameter indicates the methods to be supported for
> this session. The currently defined valid value is "PLAY". If
> not provided, the default is "PLAY". The "RECORD" value was
> defined in RFC 2326; in this specification, it is unspecified
> but reserved. RECORD and other values may be specified in the
> future.
> RFC 2326 "Real Time Streaming Protocol (RTSP)"
> (https://datatracker.ietf.org/doc/html/rfc2326), section 12.39:
>mode:
> The mode parameter indicates the methods to be supported for
> this session. Valid values are PLAY and RECORD. If not
> provided, the default is PLAY.
Please add this to teh commit message
also if you can test with anything else than gstreamer, that would be great
rtsp/rtp is often regarded as a bit picky.
Reason why iam hesitant with applying patches on this, is i am affraid fixing
it in one case could break another
also if you can check git blame, why it was wrong, and add this too to
the commit message (like this was always this way or it is a regression since)
that also adds information.
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If the United States is serious about tackling the national security threats
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
signature.asc
Description: PGP signature
___
