[FFmpeg-devel] Fix mjpeg decoder runaway from internal buffer
Anatoly Nenashev
anatoly.nenashev
Thu Oct 21 08:32:25 CEST 2010
On 21.10.2010 03:13, Michael Niedermayer wrote:
> On Wed, Oct 20, 2010 at 07:36:51PM +0400, Anatoly Nenashev wrote:
>
>> On 19.10.2010 19:14, Michael Niedermayer wrote:
>>
>>> On Tue, Oct 19, 2010 at 06:50:21PM +0400, Anatoly Nenashev wrote:
>>>
>>>
>>>> On 19.10.2010 18:31, Michael Niedermayer wrote:
>>>>
>>>>
>>>>> On Tue, Oct 19, 2010 at 05:51:55PM +0400, Anatoliy Nenashev wrote:
>>>>>
>>>>>
>>>>>
>>>>>> Hi!
>>>>>> In some cases there is a situation when mjpeg decoder runaway from
>>>>>> allocated s->buffer.
>>>>>> Usually it happens in VLC decoder for DC-AC coefficients when input
>>>>>> frame is cirrupted.
>>>>>> In this case it is caused by "specific" garbage at the end of the memory
>>>>>> allocated for s->buffer.
>>>>>>
>>>>>> Here is a fix to prevent this situation.
>>>>>>
>>>>>>
>>>>>>
>>>>> i dont see how this would prevent overreading the buffer. And no i dont
>>>>> care that on your computer with your sample this week it works.
>>>>> unless you can show that this always works (which i doubt) its not
>>>>> a correct solution.
>>>>>
>>>>>
>>>>>
>>>> 0xFF value aligned to byte is deprecated for VLC value because it is
>>>> used for markers. Thats why VLC decoder will stop within error when
>>>> intersects s->buffer_size position.
>>>>
>>>>
>>> what you write makes no sense. any VLC is allowed, 0xFF occuring
>>> in the bitstream are explicitly escaped. If i missed something in the
>>> jpeg spec that disallows such vlcs then please refer to this part of the spec
>>>
>>>
>> You are right. Sorry, it was my mistake in specification reading.
>> I found another way to fix original problem. I have added new macro in
>> get_bits.h to check up if the buffer overreaded.
>> This macro is used in mjpeg decoder. It also may be used in other decoders.
>>
>>
>>
>
>> get_bits.h | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>> 1915f32b1baadab644335e3137c93c05ad3814ac getbits.patch
>> diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
>> index f4b3646..398e0f8 100644
>> --- a/libavcodec/get_bits.h
>> +++ b/libavcodec/get_bits.h
>> @@ -124,6 +124,12 @@ LAST_SKIP_CACHE(name, gb, num)
>> LAST_SKIP_BITS(name, gb, num)
>> is equivalent to LAST_SKIP_CACHE; SKIP_COUNTER
>>
>> +INIT_OVERREAD_CHECKER(name,gb)
>> + initialize local variables for overread checker
>> +
>> +BUFFER_OVERREADED(name,gb)
>> + check if buffer overreaded
>> +
>> for examples see get_bits, show_bits, skip_bits, get_vlc
>> */
>>
>> @@ -181,6 +187,12 @@ for examples see get_bits, show_bits, skip_bits, get_vlc
>> # define GET_CACHE(name, gb)\
>> ((uint32_t)name##_cache)
>>
>> +# define INIT_OVERREAD_CHECKER(name, gb)\
>> + const int name##_size_in_bits= (gb)->size_in_bits;\
>> +
>> +# define BUFFER_OVERREADED(name, gb)\
>> + (name##_index>= name##_size_in_bits)
>> +
>> static inline int get_bits_count(const GetBitContext *s){
>> return s->index;
>> }
>> @@ -235,6 +247,12 @@ static inline void skip_bits_long(GetBitContext *s, int n){
>> # define GET_CACHE(name, gb)\
>> ((uint32_t)name##_cache)
>>
>> +# define INIT_OVERREAD_CHECKER(name, gb)\
>> + const uint8_t * name##_buffer_end= (gb)->buffer_end;\
>> +
>> +# define BUFFER_OVERREADED(name, gb)\
>> + (name##_buffer_ptr> name##_buffer_end + 1)
>> +
>> static inline int get_bits_count(const GetBitContext *s){
>> return (s->buffer_ptr - s->buffer)*8 - 16 + s->bit_count;
>> }
>> @@ -310,6 +328,12 @@ static inline void skip_bits_long(GetBitContext *s, int n){
>> # define GET_CACHE(name, gb)\
>> (name##_cache0)
>>
>> +# define INIT_OVERREAD_CHECKER(name, gb)\
>> + const uint32_t * name##_buffer_end= (gb)->buffer_end;\
>> +
>> +# define BUFFER_OVERREADED(name, gb)\
>> + (name##_buffer_ptr> name##_buffer_end + 1)
>> +
>> static inline int get_bits_count(const GetBitContext *s){
>> return ((uint8_t*)s->buffer_ptr - s->buffer)*8 - 32 + s->bit_count;
>> }
>>
>
>> mjpegdec.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>> 207d3a410e151e1e30ddaddde51449846e111c51 mjpegdec.patch
>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>> index 5686380..04f2dfe 100644
>> --- a/libavcodec/mjpegdec.c
>> +++ b/libavcodec/mjpegdec.c
>> @@ -407,6 +407,7 @@ static int decode_block(MJpegDecodeContext *s, DCTELEM *block,
>> /* AC coefs */
>> i = 0;
>> {OPEN_READER(re,&s->gb)
>> + INIT_OVERREAD_CHECKER(re,&s->gb)
>> for(;;) {
>> UPDATE_CACHE(re,&s->gb);
>> GET_VLC(code, re,&s->gb, s->vlcs[1][ac_index].table, 9, 2)
>> @@ -440,6 +441,11 @@ static int decode_block(MJpegDecodeContext *s, DCTELEM *block,
>> j = s->scantable.permutated[i];
>> block[j] = level * quant_matrix[j];
>> }
>> +
>> + if (BUFFER_OVERREADED(re,&s->gb)) {
>> + av_log(s->avctx, AV_LOG_ERROR, "buffer overreaded in decode_block\n");
>> + return -1;
>> + }
>> }
>> CLOSE_READER(re,&s->gb)}
>>
>>
> checking after each coefficient is too slow.
> Finding out how many bits a block can contain in worst case allocating
> that amount extra and then just checking once a block is simpler.
> This also should then not require these macros
>
>
This is good decision for non-broken input data. In some cases VLC
decoder can read unpredictable amount of data.
For example, let's look at the loop in function "decode_block":
for(;;) {
UPDATE_CACHE(re, &s->gb);
GET_VLC(code, re, &s->gb, s->vlcs[1][ac_index].table, 9, 2)
/* EOB */
if (code == 0x10)
break;
i += ((unsigned)code) >> 4;
if(code != 0x100){
code &= 0xf;
if(code > MIN_CACHE_BITS - 16){
UPDATE_CACHE(re, &s->gb)
}
{
int cache=GET_CACHE(re,&s->gb);
int sign=(~cache)>>31;
level = (NEG_USR32(sign ^ cache,code) ^ sign) - sign;
}
LAST_SKIP_BITS(re, &s->gb, code)
if (i >= 63) {
if(i == 63){
j = s->scantable.permutated[63];
block[j] = level * quant_matrix[j];
break;
}
av_log(s->avctx, AV_LOG_ERROR, "error count: %d\n", i);
return -1;
}
j = s->scantable.permutated[i];
block[j] = level * quant_matrix[j];
}
}
If in every loop VLC decoder returns code=0x100 than it will be infinite
loop.
Thats why we need additional check to prevent these situation.
More information about the ffmpeg-devel
mailing list