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

[Michael Niedermayer]

Hi

On Thu, Oct 19, 2006 at 08:07:46PM +0200, Baptiste Coudurier wrote:
> 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.

true


> 
> Further get_buffer/get_byte wont never fail either,

yes, unless theres somethig else wrong, but either way not a
single get_buffer() call in mov.c checks the return value so i dont see
how this would make a difference


> and further parsing
> might read memory beyond. 

i dont see how


> 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.

again i dont see how
if you say there could be some infinite loop or so, yes maybe that could
happen, though iam not sure how exactly but a security issue like writing
over the end of a buffer, i cant see how my change can cause this

but iam perfectily fine with returning -1 on any but the first call to
read_null_packet(), 
one way to do this is to set opaque to the ByteIOContext and then

if(opaque){
    opaque->opaque=NULL; 
    return buf_size;
}else
    return -1;

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is