[Ffmpeg-cvslog] r6733 - trunk/libavformat/mov.c

[Baptiste Coudurier]

Michael Niedermayer wrote:
>>>
>>>
>>> Modified: trunk/libavformat/mov.c
>>> ==============================================================================
>>> --- trunk/libavformat/mov.c	(original)
>>> +++ trunk/libavformat/mov.c	Thu Oct 19 12:05:36 2006
>>> @@ -1348,7 +1348,7 @@
>>>  #ifdef CONFIG_ZLIB
>>>  static int null_read_packet(void *opaque, uint8_t *buf, int buf_size)
>>>  {
>>> -    return -1;
>>> +    return buf_size;
>>>  }
>>>  
>> Nice it does work indeed. However isn't that dangerous ? Each time
>> fill_buffer will be called it will return buf_size and if atom.size is
>> fucked by any way (we read it from file), that will lead to memleak.
>>
>> Maybe memory should be handled differently by ByteIOContext.
> 
> i cant see any obvious memleaks which are caused by my change, so please
> elaborate how that should lead to a leak ...
> 
> also note, if the uncompress() fails then you will have a leak but that
> was already there before my change
> 
> also it seems that just duplicating some atoms will lead to a leak
> as theres no check if something has already been allocated and loaded
> its generally just a malloc() + fill array
> it should rather be if(array) fatal_error; malloc(); fill it
> 
> [...]

Humm my bad, I misused memleak term, I wanted to illustrate that if
null_read_packet always return buf_size, s->read_read_packet will never
fail and eof_reached won't ever be set.

Further get_buffer/get_byte wont never fail either, and further parsing
might read memory beyond. Since it is 'moov' atom, supposedly containing
whole metadata, I was a bit afraid that would lead to a security risk,
but I guess I'm worrying too much.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312