[FFmpeg-devel] [PATCH] check for failed extradata malloc in avidec

[Michael Niedermayer]

On Thu, Jul 02, 2009 at 05:30:21PM +0200, Reimar D?ffinger wrote:
> On Thu, Jul 02, 2009 at 04:29:46PM +0200, Michael Niedermayer wrote:
> > On Thu, Jul 02, 2009 at 02:49:59PM +0200, Reimar D?ffinger wrote:
> > > On Wed, Jul 01, 2009 at 11:27:02PM +0200, Michael Niedermayer wrote:
> > > > On Wed, Jul 01, 2009 at 08:16:56PM +0200, Reimar D?ffinger wrote:
> > > > > Hello,
> > > > > this fixes one of the files in issue1240.
> > > > > Not sure if this is the best solution (the extradata size limit of 2^30 seems
> > > > > rather extreme) but anyway...
> > > > > Index: libavformat/avidec.c
> > > > > ===================================================================
> > > > > --- libavformat/avidec.c	(revision 19317)
> > > > > +++ libavformat/avidec.c	(working copy)
> > > > > @@ -478,6 +478,10 @@
> > > > >                      if(size > 10*4 && size<(1<<30)){
> > > > >                          st->codec->extradata_size= size - 10*4;
> > > > >                          st->codec->extradata= av_malloc(st->codec->extradata_size + FF_INPUT_BUFFER_PADDING_SIZE);
> > > > > +                        if (!st->codec->extradata) {
> > > > > +                            st->codec->extradata_size= 0;
> > > > > +                            return AVERROR(ENOMEM);
> > > > > +                        }
> > > > >                          get_buffer(pb, st->codec->extradata, st->codec->extradata_size);
> > > > >                      }
> > > > 
> > > > hmm, id treat malloc failure like a too large extradata
> > > > also a error message could be usefull for both
> > > 
> > > Well, here is it done that way, I don't like it too much.
> > > And I think it creates the strange situation that on my small PC the
> > > file now plays well thanks to the failing malloc, whereas on my normal
> > > PC it would at least pointlessly try to read in the whole file (maybe
> > > we should check how many bytes get_buffer actually read an resize the
> > > extradata buffer?).
> > > I also think the limit for extradata should be at most 1 << 27, that is
> > > 128MB and would already take 2 seconds to load from an average disk (and
> > > it wouldn't fail on the average PC)...
> > 
> > i think the size of the surrounding LIST chunk (i think there is one IIRC)
> > and fsize should be used to validate the chunk sizes of the header loop.
> > if thats done, iam ok with your original variant of ENOMEM
> 
> I am not a fan of adding unrelated hacks either. Particularly doing it
> for all headers when there's no reason if there is any real world case
> where it is useful, while at the same time we have not even one piece of
> data to know how reliable the LIST value is since it was never used (not
> even by MPlayer).
> Unless you can think of a magic way of validating against the LIST
> length while still assuming that it is (in general) less reliable.
> My variant should catch missing LISTs and 0 length values for them, but
> that doesn't really seem reliable enough to me.
> Of course it could be hacked further, to e.g. only make it skip
> extradata, and only if the size is e.g. > 1MB and size is > 10* the list
> size, but for me that is just more clueless hacking around (arbitrarily
> reducing the max extradata size admittedly is, too)...
> Anyway, here is one patch that "fixes" the issue, too, in one particular
> way.

patch ok

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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-devel/attachments/20090702/49f7f89a/attachment.pgp>