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

[Michael Niedermayer]

On Sun, Apr 20, 2008 at 07:42:03PM +0200, Reimar Doeffinger wrote:
> On Sun, Apr 20, 2008 at 02:49:39PM +0200, Michael Niedermayer wrote:
> > On Sun, Apr 20, 2008 at 01:04:31PM +0200, Reimar D?ffinger 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].
> > > 
> > > Another try. I still find it ugly, but I have no really better ideas.
> > 
> > > Index: libavcodec/xiph.c
> > > ===================================================================
> > > --- libavcodec/xiph.c	(revision 12879)
> > > +++ libavcodec/xiph.c	(working copy)
> > > @@ -26,25 +26,31 @@
> > >  {
> > >      int i, j;
> > >  
> > > -    if (AV_RB16(extradata) == first_header_size) {
> > > +    if (extradata_size >= 6 && AV_RB16(extradata) == first_header_size) {
> > 
> > > +        int overall_len = 6;
> > >          for (i=0; i<3; i++) {
> > >              header_len[i] = AV_RB16(extradata);
> > >              extradata += 2;
> > >              header_start[i] = extradata;
> > >              extradata += header_len[i];
> > > +            if (overall_len > extradata_size - header_len[i])
> > > +                return -1;
> > > +            overall_len += header_len[i];
> > 
> > If you want to check for all possible cases, then you still miss one
> > 
> > uint8_t extradata_end= extradata + extradata_size;
> > for (i=0; i<3; i++) {
> >     if(   extradata_end - extradata < 2
> >        || extradata_end - extradata < AV_RB16(extradata) + 2)
> >         return -1;
> >     header_len[i] = AV_RB16(extradata);
> >     extradata += 2;
> >     header_start[i] = extradata;
> >     extradata += header_len[i];
> 
> Which case am I missing? If you mean the AV_RB16, that I think should
> be covered by the >= 6 check and overall_len being initialized to 6.

Indeed, forget what i said.


> 
> > but IMO
> > 
> >     if(extradata_end - extradata < header_len[i] + 2)
> >         return -1;
> > 
> > is enough
> 
> I don't really like leaving things "half-done". Well, that may be exaggerated, it just feels like it.

Its ironic that this line is longer than 80chars, it really should have
been halfed ;)


> 
> > >          }
> > > -    } else if (extradata[0] == 2) {
> > > +    } else if (extradata_size >= 3 && extradata_size < INT_MAX - 0x1ff && extradata[0] == 2) {
> > > +        int overall_len = 3;
> > >          for (i=0,j=1; i<2; i++,j++) {
> > >              header_len[i] = 0;
> > > -            for (; j<extradata_size && extradata[j]==0xff; j++) {
> > > +            for (; overall_len < extradata_size && extradata[j]==0xff; j++) {
> > >                  header_len[i] += 0xff;
> > > +                overall_len   += 0xff + 1;
> > >              }
> > > -            if (j >= extradata_size)
> > > +            header_len[i] += extradata[j];
> > > +            overall_len   += extradata[j];
> > > +            if (overall_len > extradata_size)
> > >                  return -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];
> > 
> > what about: ?
> > 
> > 1.change header_len to unsigned
> > 2.
> > } else if (extradata[0] == 2) {
> >     for (i=0,j=1; i<2; i++,j++) {
> >         header_len[i] = 0;
> >         for (; j<extradata_size && extradata[j]==0xff; j++) {
> >             header_len[i] += 0xff;
> >         }
> > 
> >         header_len[i] += extradata[j];
> >         if (j >= extradata_size || header_len[i] > INT_MAX/2)
> >             return -1;
> >     }
> >     if(extradata_size - j < header_len[0] + header_len[1])
> >         return -1;
> >     header_len[2] = extradata_size - header_len[0] - header_len[1] - j;
> >     extradata += j;
> >     header_start[0] = extradata;
> >     header_start[1] = header_start[0] + header_len[0];
> 
> Well, in my variant the j variable is not needed at all and can be changed to
> use *extradata++ which IMO makes things a bit less ugly again, I just didn't
> want to do it in one patch.
> Though the header_len[2] change belongs in that other patch as well actually.

Please just commit what you like best. This is such a boring bikeshed
discussion. We really both should work on more relevant things than
discussing in which order and form to place these checks.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20080420/1bbbd778/attachment.pgp>