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

[Andreas Rheinhardt]

Lynne:
> Jul 10, 2020, 14:48 by andreas.rheinhardt at gmail.com:
> 
>> 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;
>> +        }
>>  buf >>= log;
>>  buf--;
>>  skip_bits_long(gb, 32 - log);
>>
> 
> That's in an extremely hot path. Any alternatives?

If I knew a better solution, I'd have implemented it; I would also have
done the same to the codepath for the noncached bitstream reader (which
is even hotter). Notice that I actually only copied the check from the
noncached codepath (where one even has to check for log < 7 because one
might only have 25 valid bits).

There is one thing that is improvable though: The av_log(). In
situations where the return value is checked and a dedicated error
message emitted, it is unnecessary to have an av_log() here (in
particular one that logs to NULL).

- Andreas