[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