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

[Andreas Rheinhardt]

Tomas Härdin:
> 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.
> 
This function is only supposed to be used when the expected value of the
exp-golomb code is in the range 0..8190 (or other words: if there are
not more than 12 leading zeroes). And just because a specification says
that a value must be smaller than that does not mean that it is actually
fulfilled in real files.

(Btw: The cached bitstream reader version of this function can actually
handle 16 leading zeroes (and this patch only errors out if there are
more); the limit of 12 is imposed because the noncached bitstream reader
can't handle more.)

> Reading a bit about Exp-Golomb it seems counting trailing zeroes would 
> be the way to go. There's even a builtin for it, __builtin_ctz().
> Alternatively a table-driven approach could be used.
> 
Here is the code in question for the cached bitstream reader:

    buf = show_bits_long(gb, 32);

    if (buf >= (1 << 27)) {
        buf >>= 32 - 9;
        skip_bits_long(gb, ff_golomb_vlc_len[buf]);

        return ff_ue_golomb_vlc_code[buf];
    } else {
        int log = 2 * av_log2(buf) - 31;
        buf >>= log;
        buf--;
        skip_bits_long(gb, 32 - log);

        return buf;
    }

So you see we are already using a table-driven approach for small values
(namely for those values where the number of leading zeroes is <= 4) and
if we are outside of that range, an approach using av_log2 (which for
supported systems is implemented using __builtin_clz() (not trailing
zeroes!)) is used.

- Andreas