* [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
@ 2022-06-16 8:59 Thilo Borgmann
2022-06-16 9:44 ` Andreas Rheinhardt
0 siblings, 1 reply; 9+ messages in thread
From: Thilo Borgmann @ 2022-06-16 8:59 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1: Type: text/plain, Size: 134 bytes --]
Hi,
avoid left-shifting possibly negative values in case of RPU_COEFF_FLOAT. Changed RPU_COEFF_FIXED as well for consistency.
-Thilo
[-- Attachment #2: 0001-lavc-dovi_rpu-Fix-UB-for-possible-left-shift-of-nega.patch --]
[-- Type: text/plain, Size: 1528 bytes --]
From 7eca2659b588c404f058eccef478c889555957fd Mon Sep 17 00:00:00 2001
From: Michael Goulet <mgoulet@fb.com>
Date: Thu, 16 Jun 2022 10:25:28 +0200
Subject: [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative
values
It is undefined to left-shift a negative value in the RPU_COEFF_FLOAT case.
Changed the RPU_COEFF_FIXED case as well for consistency although its all positive values there.
---
libavcodec/dovi_rpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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);
--
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 8:59 [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values Thilo Borgmann
@ 2022-06-16 9:44 ` Andreas Rheinhardt
2022-06-16 10:04 ` Thilo Borgmann
0 siblings, 1 reply; 9+ messages in thread
From: Andreas Rheinhardt @ 2022-06-16 9:44 UTC (permalink / raw)
To: ffmpeg-devel
Thilo Borgmann:
> Hi,
>
> avoid left-shifting possibly negative values in case of RPU_COEFF_FLOAT.
> Changed RPU_COEFF_FIXED as well for consistency.
>
This is wrong: Both of your changes only affect RPU_COEFF_FIXED.
- Andreas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 9:44 ` Andreas Rheinhardt
@ 2022-06-16 10:04 ` Thilo Borgmann
2022-06-16 10:13 ` Andreas Rheinhardt
0 siblings, 1 reply; 9+ messages in thread
From: Thilo Borgmann @ 2022-06-16 10:04 UTC (permalink / raw)
To: ffmpeg-devel
[-- Attachment #1: Type: text/plain, Size: 483 bytes --]
Am 16.06.22 um 11:44 schrieb Andreas Rheinhardt:
> Thilo Borgmann:
>> Hi,
>>
>> avoid left-shifting possibly negative values in case of RPU_COEFF_FLOAT.
>> Changed RPU_COEFF_FIXED as well for consistency.
>>
>
> This is wrong: Both of your changes only affect RPU_COEFF_FIXED.
Indeed I'm sorry, got confused looking at the patch only when writing the commit msg.
It is actually changing get_ue_coef() for consistency although its all unsigned in there.
v2 attached.
Thanks,
Thilo
[-- Attachment #2: v2-0001-lavc-dovi_rpu-Fix-UB-for-possible-left-shift-of-n.patch --]
[-- Type: text/plain, Size: 1492 bytes --]
From 55a3d1f1d276f3327c8920ec748fed79d8a98825 Mon Sep 17 00:00:00 2001
From: Michael Goulet <mgoulet@fb.com>
Date: Thu, 16 Jun 2022 12:02:02 +0200
Subject: [PATCH v2] lavc/dovi_rpu: Fix UB for possible left shift of negative
values
It is undefined to left-shift a negative value.
Changed get_ue_coef() as well for consistency although its all positive values there.
---
libavcodec/dovi_rpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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);
--
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 10:04 ` Thilo Borgmann
@ 2022-06-16 10:13 ` Andreas Rheinhardt
2022-06-16 10:22 ` Thilo Borgmann
0 siblings, 1 reply; 9+ messages in thread
From: Andreas Rheinhardt @ 2022-06-16 10:13 UTC (permalink / raw)
To: ffmpeg-devel
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.
- Andreas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 10:13 ` Andreas Rheinhardt
@ 2022-06-16 10:22 ` Thilo Borgmann
2022-06-16 10:40 ` Andreas Rheinhardt
0 siblings, 1 reply; 9+ messages in thread
From: Thilo Borgmann @ 2022-06-16 10:22 UTC (permalink / raw)
To: ffmpeg-devel
[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]
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?
V3 attached.
Thanks,
Thilo
[-- Attachment #2: v3-0001-lavc-dovi_rpu-Fix-UB-for-possible-left-shift-of-n.patch --]
[-- Type: text/plain, Size: 1496 bytes --]
From c3e4297983f8aa74b73df1ebf48a49b3cea736a7 Mon Sep 17 00:00:00 2001
From: Michael Goulet <mgoulet@fb.com>
Date: Thu, 16 Jun 2022 12:20:10 +0200
Subject: [PATCH v3] lavc/dovi_rpu: Fix UB for possible left shift of negative
values
It is undefined to left-shift a negative value.
Changed get_ue_coef() as well for consistency although its all positive values there.
---
libavcodec/dovi_rpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
index a87562c8a3..ab14397ebd 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 * (1LL << 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 * (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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 10:22 ` Thilo Borgmann
@ 2022-06-16 10:40 ` Andreas Rheinhardt
2022-06-16 11:19 ` Thilo Borgmann
0 siblings, 1 reply; 9+ messages in thread
From: Andreas Rheinhardt @ 2022-06-16 10:40 UTC (permalink / raw)
To: ffmpeg-devel
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).)
- Andreas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 10:40 ` Andreas Rheinhardt
@ 2022-06-16 11:19 ` Thilo Borgmann
2022-06-20 9:06 ` Thilo Borgmann
0 siblings, 1 reply; 9+ messages in thread
From: Thilo Borgmann @ 2022-06-16 11:19 UTC (permalink / raw)
To: ffmpeg-devel
[-- 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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-16 11:19 ` Thilo Borgmann
@ 2022-06-20 9:06 ` Thilo Borgmann
2022-06-21 16:24 ` Thilo Borgmann
0 siblings, 1 reply; 9+ messages in thread
From: Thilo Borgmann @ 2022-06-20 9:06 UTC (permalink / raw)
To: ffmpeg-devel
Am 16.06.22 um 13:19 schrieb Thilo Borgmann:
> 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.
Pushing soon if there are no more comments.
Thanks,
Thilo
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values
2022-06-20 9:06 ` Thilo Borgmann
@ 2022-06-21 16:24 ` Thilo Borgmann
0 siblings, 0 replies; 9+ messages in thread
From: Thilo Borgmann @ 2022-06-21 16:24 UTC (permalink / raw)
To: ffmpeg-devel
Am 20.06.22 um 11:06 schrieb Thilo Borgmann:
> Am 16.06.22 um 13:19 schrieb Thilo Borgmann:
>> 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.
>
> Pushing soon if there are no more comments.
Pushed, thanks!
-Thilo
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-06-21 16:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-16 8:59 [FFmpeg-devel] [PATCH] lavc/dovi_rpu: Fix UB for possible left shift of negative values Thilo Borgmann
2022-06-16 9:44 ` Andreas Rheinhardt
2022-06-16 10:04 ` Thilo Borgmann
2022-06-16 10:13 ` Andreas Rheinhardt
2022-06-16 10:22 ` Thilo Borgmann
2022-06-16 10:40 ` Andreas Rheinhardt
2022-06-16 11:19 ` Thilo Borgmann
2022-06-20 9:06 ` Thilo Borgmann
2022-06-21 16:24 ` Thilo Borgmann
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git