[FFmpeg-devel] Fix mjpeg decoder runaway from internal buffer
Michael Niedermayer
michaelni
Fri Oct 22 00:54:05 CEST 2010
On Thu, Oct 21, 2010 at 10:32:25AM +0400, Anatoly Nenashev wrote:
> 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.
ive fixed that, actually this case was possibly exploitable as the i>=63
is a signed check and i can overflow that way
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/20101022/2123f9c8/attachment.pgp>
More information about the ffmpeg-devel
mailing list