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

[Sean McGovern <gseanmcg@gmail.com>]

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