[FFmpeg-cvslog] r16966 - trunk/libavformat/aiff.c

[Michael Niedermayer]

On Mon, Feb 02, 2009 at 04:32:30PM -0800, Baptiste Coudurier wrote:
> Aurelien Jacobs wrote:
> > Baptiste Coudurier wrote:
> > 
> >> Aurelien Jacobs wrote:
> >>> Baptiste Coudurier wrote:
> >>>
> >>>> aurel wrote:
> >>>>> Author: aurel
> >>>>> Date: Tue Feb  3 00:43:12 2009
> >>>>> New Revision: 16966
> >>>>>
> >>>>> Log:
> >>>>> use new metadata API in aiff demuxer
> >>>>>
> >>>>> Modified:
> >>>>>    trunk/libavformat/aiff.c
> >>>>>
> >>>>> Modified: trunk/libavformat/aiff.c
> >>>>> ==============================================================================
> >>>>> --- trunk/libavformat/aiff.c	Tue Feb  3 00:37:03 2009	(r16965)
> >>>>> +++ trunk/libavformat/aiff.c	Tue Feb  3 00:43:12 2009	(r16966)
> >>>>> @@ -79,9 +79,10 @@ static int get_tag(ByteIOContext *pb, ui
> >>>>>  }
> >>>>>  
> >>>>>  /* Metadata string read */
> >>>>> -static void get_meta(ByteIOContext *pb, char * str, int strsize, int size)
> >>>>> +static void get_meta(AVFormatContext *s, const char *key, int size)
> >>>>>  {
> >>>>> -    int res = get_buffer(pb, (uint8_t*)str, FFMIN(strsize-1, size));
> >>>>> +    uint8_t str[1024];
> >>>>> +    int res = get_buffer(s->pb, str, FFMIN(sizeof(str)-1, size));
> >>>>>      if (res < 0)
> >>>>>          return;
> >>>> Does the new API have the 1024 limitation ? I hope not.
> >>> Hopefully not !
> >>>
> >>>> Therefore everything must be read and it's up the compat layer to
> >>>> truncate it to 1024.
> >>> Everything ? size is a 32 byte value read directly from the bitstream,
> >>> so I would rather not try to av_malloc(size).
> >>> So IMO some sanity check must be applied. I though 1024 would be plenty
> >>> enough. How much do you think the limitation should be ?
> >> You have a "comment" field, what if quicktime reads it all ?
> > 
> > If quicktime wants to read a full 1GB comment field, that is it's own
> > problem. I don't think FFmpeg should do this...
> 
> Well you can get users complaining...
> Though I also think it's not reasonable.
> 
> >> What if I want to write my lyrics in there ? :>
> > 
> > Then you would probably need a bit more than 1024 bytes...
> > If you think it's useful, I will av_malloc() the buffer,
> > but I still need to set some limit. Would a 1MB limit
> > be reasonable enough ?
> 
> To be honest, I don't really know, I guess 1MB is ok, I'd like to get
> others opinion also.

we may need some user settable limit, a file with huge metadata might be too
much for some embeded system but users on a desktop might still want it
all
also we may need such a limit for demuxers, the same problem exists for
frames, currently some demuxers (avi) implement checks like checking sizes
against the filesize, this also might be a good idea for other demuxers
and we should add it to our patch checklist. (code must not malloc
any arbitrary amounts due to crafted input data due to OOM/DOS issues)

also see max_index_size / max_picture_buffer for similar variables which
we alraedy have

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20090203/3f20782d/attachment.pgp>