[FFmpeg-devel] [PATCH] avcodec/golomb: Prevent shift by negative number

[Andreas Rheinhardt]

Michael Niedermayer:
> On Tue, Jul 14, 2020 at 06:10:39PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Mon, Jul 13, 2020 at 09:04:30PM +0200, Tomas Härdin wrote:
>>>> fre 2020-07-10 klockan 15:48 +0200 skrev Andreas Rheinhardt:
>>>>> This happened in get_ue_golomb() if the cached bitstream reader was
>>>>> in
>>>>> use, because there was no check to handle the case of the read value
>>>>> not being in the range 0..8190.
>>>>>
>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>>>>> ---
>>>>>  libavcodec/golomb.h | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
>>>>> index 7fd46a91bd..5bfcfe085f 100644
>>>>> --- a/libavcodec/golomb.h
>>>>> +++ b/libavcodec/golomb.h
>>>>> @@ -66,6 +66,10 @@ static inline int get_ue_golomb(GetBitContext *gb)
>>>>>          return ff_ue_golomb_vlc_code[buf];
>>>>>      } else {
>>>>>          int log = 2 * av_log2(buf) - 31;
>>>>> +        if (log < 0) {
>>>>> +            av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
>>>>> +            return AVERROR_INVALIDDATA;
>>>>> +        }
>>>>
>>>> How come invalid codes can even be present like this? Seems
>>>> inefficient. Or is the decoder wrong? Maybe Michael wants to chime in
>>>> here, since he wrote the original implementation.
>>>
>>> The codepath of the non cached one does a check too. It should be
>>> done consistently.
>>
>> Does this imply that you want me to error out for log < 7 although the
>> cached bitstream reader is able to read the number even when log is in
>> the range of 0..6? (The reason for this is that one only gets 25 valid
>> bits in the noncached version.)
> 
> is it intended to be valid for this function to be used for longer codes ?
> if not it should fail because otherwise a bug in the code would be missed
> also it would result in fewer differences and easier testing as both
> would behave the same
> 
Ok, will change locally.

- Andreas