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

[Michael Niedermayer]

On Tue, Apr 15, 2008 at 08:40:19PM +0200, Reimar D?ffinger wrote:
> On Tue, Apr 15, 2008 at 08:10:38PM +0200, Michael Niedermayer wrote:
> > On Tue, Apr 15, 2008 at 07:44:49PM +0200, Reimar D?ffinger wrote:
> > > On Tue, Apr 15, 2008 at 07:38:57PM +0200, Reimar D?ffinger wrote:
> > > > 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:
> > > > > > 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.
> > > 
> > > With better aligning.
> > 
> > > 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 - (0xff + 1))
> > > +                    return -1;
> > > +                overall_len   += 0xff + 1;
> > >                  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];
> > 
> > int overall_len = 1;
> > for (i=0,j=1; i<2; i++,j++) {
> >     header_len[i] = 0;
> >     for (; overall_len <= extradata_size && extradata[j]==0xff; j++) {
> >         overall_len   += 0xff + 1;
> >         header_len[i] += 0xff;
> >     }
> >     overall_len   += extradata[j];
> 
> I assume you forgot the +1 here?

no, look up "int overall_len = 1"


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20080415/1e3ee2bf/attachment.pgp>