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

[Reimar Doeffinger]

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.

> 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.

> >          }
> > -    } 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.