[FFmpeg-devel] Fix mjpeg decoder runaway from internal buffer

Anatoly Nenashev anatoly.nenashev
Thu Oct 21 14:42:00 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.
>    
No it doesn't too slow.

I've made some tests:

1) Creating test video
ffmpeg -an -i big_buck_bunny_1080p_h264.mov -vcodec mjpeg -qmax 2 
big_buck_bunny_1080p_mjpeg.mov

2) Decoding MJPEG movie without overreading check patch.
  time ./ffmpeg -y -i 
/media/DataStorage/Films/big_buck_bunny_1080p_mjpeg.mov -f rawvideo 
/dev/null
...
frame=14315 fps= 36 q=0.0 Lsize=       0kB time=596.46 bitrate=   
0.0kbits/s
video:43481812kB audio:0kB global headers:0kB muxing overhead -100.000000%

real    6m35.055s
user    5m28.238s
sys    1m0.141s

3) Decoding MJPEG movie with overreading check patch.
  time ./ffmpeg -y -i 
/media/DataStorage/Films/big_buck_bunny_1080p_mjpeg.mov -f rawvideo 
/dev/null
...
frame=14315 fps= 51 q=0.0 Lsize=       0kB time=596.46 bitrate=   
0.0kbits/s
video:43481812kB audio:0kB global headers:0kB muxing overhead -100.000000%

real    6m37.744s
user    5m29.503s
sys    1m0.339s



System info: CPU Intel Core 2 Duo SpeedStep disabled in BIOS, OS Linux 
Debian squeeze, kernel 2.6.35 BFS, run in single user mode.



More information about the ffmpeg-devel mailing list