Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values

[Thilo Borgmann <thilo.borgmann@mail.de>]

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

Am 16.06.22 um 12:40 schrieb Andreas Rheinhardt:
> Thilo Borgmann:
>> Am 16.06.22 um 12:13 schrieb Andreas Rheinhardt:
>>> Thilo Borgmann:
>>>> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
>>>> index a87562c8a3..833ce9e705 100644
>>>> --- a/libavcodec/dovi_rpu.c
>>>> +++ b/libavcodec/dovi_rpu.c
>>>> @@ -153,7 +153,7 @@ static inline uint64_t get_ue_coef(GetBitContext
>>>> *gb, const AVDOVIRpuDataHeader
>>>>        case RPU_COEFF_FIXED:
>>>>            ipart = get_ue_golomb_long(gb);
>>>>            fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
>>>> -        return (ipart << hdr->coef_log2_denom) + fpart.u32;
>>>> +        return ipart * (1 << hdr->coef_log2_denom) + fpart.u32;
>>>>          case RPU_COEFF_FLOAT:
>>>>            fpart.u32 = get_bits_long(gb, 32);
>>>> @@ -172,7 +172,7 @@ static inline int64_t get_se_coef(GetBitContext
>>>> *gb, const AVDOVIRpuDataHeader *
>>>>        case RPU_COEFF_FIXED:
>>>>            ipart = get_se_golomb_long(gb);
>>>>            fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
>>>> -        return (ipart << hdr->coef_log2_denom) + fpart.u32;
>>>> +        return ipart * (1 << hdr->coef_log2_denom) + fpart.u32;
>>>>          case RPU_COEFF_FLOAT:
>>>>            fpart.u32 = get_bits_long(gb, 32);
>>>
>>> coef_log2_denom can be in the range 13..32. This means that 1 <<
>>> hdr->coef_log2_denom can be UB (namely if coef_log2_denom is 31 or 32
>>> for ordinary 32 bit ints); this time it is not UB that happens to work
>>> as expected, because 1 << 32 will be 0 or 1 (depending upon the system)
>>> and not 2^32. In case of get_ue_coef() this actually adds UB to
>>> otherwise fine code.
>>
>> So 1LL it needs to be, not? Am I still missing something?
>>
> 
> This version should not add new UB.
> I btw don't get why you are changing get_ue_coef() at all: It's fine
> as-is; consistency should only trump when the choice is between several
> equally good alternatives which is IMO not true here. (The reason that C
> makes left shifts of negative values UB is because of non-two's
> complement systems, so it is unfortunate for a project like FFmpeg that
> requires two's complement that it has to workaround this restriction by
> using a * (1 << b).)

Just overthinking consistency, I guess.

v4 changes get_se_coef() only.

Thanks,
Thilo

[-- Attachment #2: v4-0001-lavc-dovi_rpu-Fix-UB-for-possible-left-shift-of-n.patch --]
[-- Type: text/plain, Size: 975 bytes --]

From 85429be17bf238642aeacf93e4d7c16d4c4b03a3 Mon Sep 17 00:00:00 2001
From: Michael Goulet <mgoulet@fb.com>
Date: Thu, 16 Jun 2022 13:17:25 +0200
Subject: [PATCH v4] lavc/dovi_rpu: Fix UB for possible left shift of negative
 values

It is undefined to left-shift a negative value.
---
 libavcodec/dovi_rpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
index a87562c8a3..dd38936552 100644
--- a/libavcodec/dovi_rpu.c
+++ b/libavcodec/dovi_rpu.c
@@ -172,7 +172,7 @@ static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader *
     case RPU_COEFF_FIXED:
         ipart = get_se_golomb_long(gb);
         fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
-        return (ipart << hdr->coef_log2_denom) + fpart.u32;
+        return ipart * (1LL << hdr->coef_log2_denom) + fpart.u32;
 
     case RPU_COEFF_FLOAT:
         fpart.u32 = get_bits_long(gb, 32);
-- 
2.20.1 (Apple Git-117)


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