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

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

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