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

[Michael Niedermayer]

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];

but IMO

    if(extradata_end - extradata < header_len[i] + 2)
        return -1;

is enough



>          }
> -    } 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];

anyway, feel free to commit what you like best ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/9aca7e94/attachment.pgp>