| Message ID | CAPBf_OmQn7OdaSg7mkWEtieUgVBW95oQqY9kZEzQppRpS3rd-Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,RFC] fix UB in fate-checkasm-sw_yuv2rgb | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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".
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