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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101021/318ccaef/attachment.pgp>