[FFmpeg-devel] [RFC] fix UB in fate-checkasm-sw_yuv2rgb

[FFmpeg-devel] [RFC] fix UB in fate-checkasm-sw_yuv2rgb

[Sean McGovern]

[-- Attachment #1: Type: text/plain, Size: 175 bytes --]

Hi,

Attached is an RFC patch to address the undefined behaviour from the
new `fate-checkasm-sw_yuv2rgb` test seen on both the x86 and ppc UBSan
FATE nodes.

-- Sean McGovern

[-- Attachment #2: 0001-swscale-prevent-undefined-behaviour-in-the-PUTRGBA-m.patch --]
[-- Type: text/x-patch, Size: 1993 bytes --]

From 7b7c5fe69443085250ce8fc3511dddd0cfa2d756 Mon Sep 17 00:00:00 2001
From: Sean McGovern <gseanmcg@gmail.com>
Date: Tue, 2 Jul 2024 23:07:54 -0400
Subject: [RFC PATCH] swscale: prevent undefined behaviour in the PUTRGBA macro

---

Notes:
    Sending this as an RFC as I'm not sure it is the
    correct fix.
    
    It does address the undefined behaviour of the C version of yuv2rgb
    tested in 'fate-checkasm-sw_yuv2rgb', but since swscale new territory for me
    I'm not sure what I propose is appropriate.
    
    I think the AltiVec version will still need a fix after this, and Ramiro
    suggested there might be an issue in the LoongArch version as well.
    
    Conversation points:
    
    - Is usage of '__typeof__' OK? Is it a GCC-ism?
    In the rest of the codebase it seems to be limited to AltiVec acceleration.
    - Should this instead just cast the shifted arguments to 'int32_t' and
    be done with it?
    
    Aside: the macro soup in this file has very high cognitive complexity.

 libswscale/yuv2rgb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libswscale/yuv2rgb.c b/libswscale/yuv2rgb.c
index 977eb3a7dd..ab5192aab4 100644
--- a/libswscale/yuv2rgb.c
+++ b/libswscale/yuv2rgb.c
@@ -100,9 +100,9 @@ const int *sws_getCoefficients(int colorspace)
 
 #define PUTRGBA(dst, ysrc, asrc, i, abase)                              \
     Y              = ysrc[2 * i];                                       \
-    dst[2 * i]     = r[Y] + g[Y] + b[Y] + (asrc[2 * i]     << abase);   \
+    dst[2 * i]     = r[Y] + g[Y] + b[Y] + ((__typeof__(*dst))(asrc[2 * i])     << abase);   \
     Y              = ysrc[2 * i + 1];                                   \
-    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + (asrc[2 * i + 1] << abase);
+    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + ((__typeof__(*dst))(asrc[2 * i + 1]) << abase);
 
 #define PUTRGB48(dst, src, asrc, i, abase)          \
     Y                = src[ 2 * i];                 \
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [RFC] fix UB in fate-checkasm-sw_yuv2rgb

[Rémi Denis-Courmont]

Le keskiviikkona 3. heinäkuuta 2024, 22.07.42 EEST Sean McGovern a écrit :
> Hi,
> 
> Attached is an RFC patch to address the undefined behaviour from the
> new `fate-checkasm-sw_yuv2rgb` test seen on both the x86 and ppc UBSan
> FATE nodes.
> 
> -- Sean McGovern

__typeof__ is a GCCism. C23 has typeof() which is pretty much the same with a 
more legible name. But neither are OK at this point in FFmpeg anyhow.

-- 
レミ・デニ-クールモン
http://www.remlab.net/



_______________________________________________
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".

Re: [FFmpeg-devel] [RFC] fix UB in fate-checkasm-sw_yuv2rgb

[Sean McGovern]

Hi Rémi,

On Wed, Jul 3, 2024 at 4:34 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>
> Le keskiviikkona 3. heinäkuuta 2024, 22.07.42 EEST Sean McGovern a écrit :
> > Hi,
> >
> > Attached is an RFC patch to address the undefined behaviour from the
> > new `fate-checkasm-sw_yuv2rgb` test seen on both the x86 and ppc UBSan
> > FATE nodes.
> >
> > -- Sean McGovern
>
> __typeof__ is a GCCism. C23 has typeof() which is pretty much the same with a
> more legible name. But neither are OK at this point in FFmpeg anyhow.

OK, fair enough. Should I just cast them to int32_t instead?

>
> --
> レミ・デニ-クールモン
> http://www.remlab.net/
>
>
>
> _______________________________________________
> 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".
_______________________________________________
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".

[FFmpeg-devel] [PATCHv2] swscale: prevent undefined behaviour in the PUTRGBA macro

[Sean McGovern]

---
 libswscale/yuv2rgb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libswscale/yuv2rgb.c b/libswscale/yuv2rgb.c
index 977eb3a7dd..ac0b811f61 100644
--- a/libswscale/yuv2rgb.c
+++ b/libswscale/yuv2rgb.c
@@ -100,9 +100,9 @@ const int *sws_getCoefficients(int colorspace)
 
 #define PUTRGBA(dst, ysrc, asrc, i, abase)                              \
     Y              = ysrc[2 * i];                                       \
-    dst[2 * i]     = r[Y] + g[Y] + b[Y] + (asrc[2 * i]     << abase);   \
+    dst[2 * i]     = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i])     << abase);   \
     Y              = ysrc[2 * i + 1];                                   \
-    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + (asrc[2 * i + 1] << abase);
+    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i + 1]) << abase);
 
 #define PUTRGB48(dst, src, asrc, i, abase)          \
     Y                = src[ 2 * i];                 \
-- 
2.39.2

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCHv2] swscale: prevent undefined behaviour in the PUTRGBA macro

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1243 bytes --]

On Mon, Jul 08, 2024 at 12:25:17AM -0400, Sean McGovern wrote:
> ---
>  libswscale/yuv2rgb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libswscale/yuv2rgb.c b/libswscale/yuv2rgb.c
> index 977eb3a7dd..ac0b811f61 100644
> --- a/libswscale/yuv2rgb.c
> +++ b/libswscale/yuv2rgb.c
> @@ -100,9 +100,9 @@ const int *sws_getCoefficients(int colorspace)
>  
>  #define PUTRGBA(dst, ysrc, asrc, i, abase)                              \
>      Y              = ysrc[2 * i];                                       \
> -    dst[2 * i]     = r[Y] + g[Y] + b[Y] + (asrc[2 * i]     << abase);   \
> +    dst[2 * i]     = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i])     << abase);   \
>      Y              = ysrc[2 * i + 1];                                   \
> -    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + (asrc[2 * i + 1] << abase);
> +    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i + 1]) << abase);

can you explain what undefined behavior this does prevent and how ?
(in the commit message)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

[FFmpeg-devel] [PATCHv3] swscale: prevent undefined behaviour in the PUTRGBA macro

[Sean McGovern]

For even small values of 'asrc', shifting them by 24 bits or more
will cause arithmetic overflow and be caught by
GCC's undefined behaviour sanitizer.

Ensure the values do not overflow by up-casting the bracketed
expressions involving 'asrc' to int32_t.
---
 libswscale/yuv2rgb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libswscale/yuv2rgb.c b/libswscale/yuv2rgb.c
index 977eb3a7dd..ac0b811f61 100644
--- a/libswscale/yuv2rgb.c
+++ b/libswscale/yuv2rgb.c
@@ -100,9 +100,9 @@ const int *sws_getCoefficients(int colorspace)
 
 #define PUTRGBA(dst, ysrc, asrc, i, abase)                              \
     Y              = ysrc[2 * i];                                       \
-    dst[2 * i]     = r[Y] + g[Y] + b[Y] + (asrc[2 * i]     << abase);   \
+    dst[2 * i]     = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i])     << abase);   \
     Y              = ysrc[2 * i + 1];                                   \
-    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + (asrc[2 * i + 1] << abase);
+    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i + 1]) << abase);
 
 #define PUTRGB48(dst, src, asrc, i, abase)          \
     Y                = src[ 2 * i];                 \
-- 
2.39.2

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCHv3] swscale: prevent undefined behaviour in the PUTRGBA macro

[Leo Izen]

On 7/9/24 2:34 PM, Sean McGovern wrote:
> For even small values of 'asrc', shifting them by 24 bits or more
> will cause arithmetic overflow and be caught by
> GCC's undefined behaviour sanitizer.
> 
> Ensure the values do not overflow by up-casting the bracketed
> expressions involving 'asrc' to int32_t.
> ---
>   libswscale/yuv2rgb.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libswscale/yuv2rgb.c b/libswscale/yuv2rgb.c
> index 977eb3a7dd..ac0b811f61 100644
> --- a/libswscale/yuv2rgb.c
> +++ b/libswscale/yuv2rgb.c
> @@ -100,9 +100,9 @@ const int *sws_getCoefficients(int colorspace)
>   
>   #define PUTRGBA(dst, ysrc, asrc, i, abase)                              \
>       Y              = ysrc[2 * i];                                       \
> -    dst[2 * i]     = r[Y] + g[Y] + b[Y] + (asrc[2 * i]     << abase);   \
> +    dst[2 * i]     = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i])     << abase);   \
>       Y              = ysrc[2 * i + 1];                                   \
> -    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + (asrc[2 * i + 1] << abase);
> +    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + ((int32_t)(asrc[2 * i + 1]) << abase);
>   
>   #define PUTRGB48(dst, src, asrc, i, abase)          \
>       Y                = src[ 2 * i];                 \

Are these able to be negative? If not, it may be wise to cast to 
uint32_t instead as left shifting a negative integer is UB.

- Leo Izen (Traneptora)
_______________________________________________
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".

[FFmpeg-devel] [PATCHv4] swscale: prevent undefined behaviour in the PUTRGBA macro

[Sean McGovern]

For even small values of 'asrc[x]', shifting them by 24 bits or more
will cause arithmetic overflow and be caught by
GCC's undefined behaviour sanitizer.

Ensure the values do not overflow by up-casting the bracketed
expressions involving 'asrc' to uint32_t.
---
 libswscale/yuv2rgb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libswscale/yuv2rgb.c b/libswscale/yuv2rgb.c
index 977eb3a7dd..cfbc54abd0 100644
--- a/libswscale/yuv2rgb.c
+++ b/libswscale/yuv2rgb.c
@@ -100,9 +100,9 @@ const int *sws_getCoefficients(int colorspace)
 
 #define PUTRGBA(dst, ysrc, asrc, i, abase)                              \
     Y              = ysrc[2 * i];                                       \
-    dst[2 * i]     = r[Y] + g[Y] + b[Y] + (asrc[2 * i]     << abase);   \
+    dst[2 * i]     = r[Y] + g[Y] + b[Y] + ((uint32_t)(asrc[2 * i])     << abase);   \
     Y              = ysrc[2 * i + 1];                                   \
-    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + (asrc[2 * i + 1] << abase);
+    dst[2 * i + 1] = r[Y] + g[Y] + b[Y] + ((uint32_t)(asrc[2 * i + 1]) << abase);
 
 #define PUTRGB48(dst, src, asrc, i, abase)          \
     Y                = src[ 2 * i];                 \
-- 
2.39.2

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCHv4] swscale: prevent undefined behaviour in the PUTRGBA macro

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 595 bytes --]

On Tue, Jul 09, 2024 at 05:41:32PM -0400, Sean McGovern wrote:
> For even small values of 'asrc[x]', shifting them by 24 bits or more
> will cause arithmetic overflow and be caught by
> GCC's undefined behaviour sanitizer.
> 
> Ensure the values do not overflow by up-casting the bracketed
> expressions involving 'asrc' to uint32_t.
> ---
>  libswscale/yuv2rgb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

will apply

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".