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

Reimar Döffinger Reimar.Doeffinger
Fri Jul 3 13:28:35 CEST 2009


On Thu, Jul 02, 2009 at 08:10:40PM +0200, Michael Niedermayer wrote:
> 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

This and the very first patch applied.



More information about the ffmpeg-devel mailing list