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

[Reimar Döffinger]

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.
Index: libavformat/avidec.c
===================================================================
--- libavformat/avidec.c        (revision 19325)
+++ libavformat/avidec.c        (working copy)
@@ -252,6 +252,7 @@
     AVIStream *ast = NULL;
     int avih_width=0, avih_height=0;
     int amv_file_format=0;
+    uint64_t list_end = 0;
 
     avi->stream_index= -1;
 
@@ -277,6 +278,7 @@
 
         switch(tag) {
         case MKTAG('L', 'I', 'S', 'T'):
+            list_end = url_ftell(pb) + size;
             /* Ignored, except at start of video packets. */
             tag1 = get_le32(pb);
 #ifdef DEBUG
@@ -445,6 +447,9 @@
             if (stream_index >= (unsigned)s->nb_streams || avi->dv_demux) {
                 url_fskip(pb, size);
             } else {
+                uint64_t cur_pos = url_ftell(pb);
+                if (cur_pos < list_end)
+                    size = FFMIN(size, list_end - cur_pos);
                 st = s->streams[stream_index];
                 switch(codec_type) {
                 case CODEC_TYPE_VIDEO: