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

[Michael Niedermayer]

Hi

On Fri, Oct 20, 2006 at 10:43:58AM +0200, Baptiste Coudurier wrote:
> Hi
> 
> Michael Niedermayer wrote:
> >>>
> >>> [...]
> >> 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
> 
> I think difference is that we read from file, while here we read from
> memory.

there is a fixed sized buffer, i dont see how you will ever read from
outside that, and the inside of it is the decompressed header ...


> 
> >> and further parsing
> >> might read memory beyond. 
> > 
> > i dont see how
> > 
> 
> Let's say, stts atom contains a wrong entry count, it will read process'
> memory after the moov_data allocated, and nothing will stop. While
> reading from file is not really harmful, I feel like more worried about
> reading from memory.
> 
> >> 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
> 
> Not directly, and Im surely wrong being afraid.
> 
> > 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;
> > 
> > [...]
> 
> I was thinking about something like that, or init_put_bytes with
> read_packet as NULL, and internally use a null_read_packet(). That way,
> you don't have to define a null_reading_packet each time you want to
> read from memory.

if you want a clean implementation then there should be no
null_reading_packet() but the pointer should always be NULL and never
be called

[...]

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