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

[Andreas Rheinhardt]

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

> If the check causes a meassurable speedloss, then it can be implemented
> without a check. For example by using asm for the shift operation, or maybe
> compilers have some nicer function doing a never undefined shift. If such
> function does not exist, its something that maybe someone should add to
> gcc & clang because other projects could benefit from it too in similar
> situations.

22e960ad478e568f4094971a58c6ad8f549c0180 (which enabled the check and
added the av_log() in the noncached branch) claims no measurable speedloss.
I have not found any such compiler builtin function for this purpose;
neither GCC nor Clang have one.
Notice that if we had such an always defined shift, we could use it to
make put_bits() write 0-32 bits at once if this always defined shift had
the property that 0U << 32 equals 0.

- Andreas