[FFmpeg-devel] [PATCH V2] lavc/golomb: Fix UE golomb overwrite issue.

[Jun Zhao]

On 2017/6/7 9:22, Michael Niedermayer wrote:
> On Mon, Jun 05, 2017 at 08:43:35AM +0800, Jun Zhao wrote:
>> V2: Add Add set_ue_golomb_long() to support 32bits UE golomb and update the unit test.
> 
>>  golomb.h       |   20 +++++++++++++++++++-
>>  put_bits.h     |   35 +++++++++++++++++++++++++++++++++++
>>  tests/golomb.c |   19 +++++++++++++++++++
>>  3 files changed, 73 insertions(+), 1 deletion(-)
>> 491565dd491fc4ebd1717069d9c7655bfe0bd08a  0001-lavc-golomb-Fix-UE-golomb-overwrite-issue.patch
>> From 6fe36e4e2a41f70e2a41c5eba90b5143b4eeba7b Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao at intel.com>
>> Date: Fri, 2 Jun 2017 15:05:49 +0800
>> Subject: [PATCH V2] lavc/golomb: Fix UE golomb overwrite issue.
>>
>> put_bits just support write up to 31 bits, when write 32 bit in
>> put_bits, it's will overwrite the bit buffer, because the default
>> assert level is 0, the av_assert2(n <= 31 && value < (1U << n))
>> in put_bits can not be trigger runtime. Add set_ue_golomb_long()
>> to support 32bits UE golomb.
>>
>> Signed-off-by: Jun Zhao <jun.zhao at intel.com>
>> ---
>>  libavcodec/golomb.h       | 20 +++++++++++++++++++-
>>  libavcodec/put_bits.h     | 35 +++++++++++++++++++++++++++++++++++
>>  libavcodec/tests/golomb.c | 19 +++++++++++++++++++
>>  3 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
>> index 0833aff468..47ab884282 100644
>> --- a/libavcodec/golomb.h
>> +++ b/libavcodec/golomb.h
>> @@ -458,7 +458,7 @@ static inline int get_te(GetBitContext *s, int r, char *file, const char *func,
>>  #endif /* TRACE */
>>  
>>  /**
>> - * write unsigned exp golomb code.
>> + * write unsigned exp golomb code. 2^16-2 at most.
>>   */
>>  static inline void set_ue_golomb(PutBitContext *pb, int i)
>>  {
>> @@ -473,6 +473,24 @@ static inline void set_ue_golomb(PutBitContext *pb, int i)
>>  }
>>  
>>  /**
>> + * write unsigned exp golomb code. 2^32-2 at most.
>> + */
>> +static inline void set_ue_golomb_long(PutBitContext *pb, uint32_t i)
>> +{
>> +    av_assert2(i <= (0xffffffff - 2));
>> +
>> +    if (i < 256)
>> +        put_bits(pb, ff_ue_golomb_len[i], i + 1);
>> +    else {
> 
> Please add {} for if else so its if { } else
> 

Ok, will add {} for if.

>> +        int e = av_log2(i + 1);
>> +        if (e < 16)
>> +            put_bits(pb, 2 * e + 1, i + 1);
>> +        else
> 
>> +            put_bits64(pb, 2 * e + 1, i + 1);
> 
> put_bits64 tests for <32 it tests for ==64 neither are possible
> here. And this is a inline function so these impossible code pathes
> might get duplicated many times
> 
> [...]

I think av_assert2(i <= (0xffffffff - 2)) have cover this condition, and maybe
av_assert0(i <= (0xffffffff - 2)) is a better choice for this assert.

> 
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>