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

[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?

V3 attached.

Thanks,
Thilo
-------------- next part --------------
From c3e4297983f8aa74b73df1ebf48a49b3cea736a7 Mon Sep 17 00:00:00 2001
From: Michael Goulet <mgoulet at 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)