| Message ID | ce5cd551-71db-8cfa-df92-e63b78450d05@mail.de |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/dovi_rpu: Fix UB for possible left shift of negative values | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
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
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 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);
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
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 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);
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
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 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);
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
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
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);
Hi, avoid left-shifting possibly negative values in case of RPU_COEFF_FLOAT. Changed RPU_COEFF_FIXED as well for consistency. -Thilo 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(-)