[FFmpeg-devel] [PATCH] ff_split_xiph_headers returns broken header_len < 0

[Reimar Döffinger]

On Tue, Apr 15, 2008 at 07:11:12PM +0200, Michael Niedermayer wrote:
> On Tue, Apr 15, 2008 at 06:54:45PM +0200, Reimar D?ffinger wrote:
> > Hello,
> > when trying to play http://wdz5.xs4all.nl/~hendrik/mmw-deadzy.ogg with
> > MPlayer (ffplay untested), the vorbis decoder crashes.
> > The reason is that ff_split_xiph_headers does not fail but returns an
> > invalid (negative) header_len[2].
> > Attached patch is one possible fix. Maybe doing a return -1 if this case
> > happens is the better solution, I just like to default to solutions that
> > have a higher chance of still working with broken files (though at least
> > in this case it does not help anyway, the files till does not play).
> 
> I prefer a return -1 (unless doing something else helps some existing file)
> Also your check is insufficient header_len 0/1 still can reach arbitrary
> values, i think this should be fixed more generically. That is checking
> that all are within extradata.

I misread one of the existing checks, I think attached patch should
actually work.
-------------- next part --------------
Index: libavcodec/xiph.c
===================================================================
--- libavcodec/xiph.c	(revision 12807)
+++ libavcodec/xiph.c	(working copy)
@@ -34,17 +34,24 @@
             extradata += header_len[i];
         }
     } else if (extradata[0] == 2) {
+        int overall_len = 0;
         for (i=0,j=1; i<2; i++,j++) {
             header_len[i] = 0;
             for (; j<extradata_size && extradata[j]==0xff; j++) {
+                if (overall_len > extradata_size - 0x100)
+                    return -1;
+                overall_len += 0x100;
                 header_len[i] += 0xff;
             }
             if (j >= extradata_size)
                 return -1;
 
+            if (overall_len > extradata_size - (extradata[j] + 1))
+                return -1;
+            overall_len += extradata[j] + 1;
             header_len[i] += extradata[j];
         }
-        header_len[2] = extradata_size - header_len[0] - header_len[1] - j;
+        header_len[2] = extradata_size - overall_len;
         extradata += j;
         header_start[0] = extradata;
         header_start[1] = header_start[0] + header_len[0];