[FFmpeg-devel] [PATCH] Electronic Arts TGV decoder

[Michael Niedermayer]

On Wed, Jul 09, 2008 at 09:18:13PM +1000, pross at xvid.org wrote:
> Hi!
> 
> Second video codec in the EA series.
> 
> Samples: http://samples.mplayerhq.hu/game-formats/ea-tgv/
> Write-up: http://wiki.multimedia.cx/index.php?title=Electronic_Arts_TGV
[...]

> +
> +/**
> + * Unpack buffer
> + * @return 0 on success, -1 on critical buffer underflow
> + */
> +static int unpack(const uint8_t *src, const uint8_t *src_end, unsigned char *dst, int width, int stride) {
> +    int size1,size2,offset;
> +    int size;
> +    int i;
> +    int o = 0;
> +

> +    if (src+2>src_end)
> +        return -1;
> +    if ((AV_RB16(&src[0]) & 0x0100))
> +        src += 5;
> +    else
> +        src += 2;
> +
> +    if (src+4>src_end)
> +        return -1;
> +    size = AV_RB24(&src[0]);
> +    src += 3;

the first end check seems unneeded, and reading a byte would be enough for
the if()



> +
> +    while(size>0 && src<src_end) {
> +
> +        /* determine size1 and size2 */
> +        if ( src[0] & 0x80 ) {  // 1
> +            if (src[0] & 0x40 ) {  // 11
> +                if ( src[0] & 0x20 ) {  // 111

> +                    if ( src[0] >= 0xFC ) {  // 111111
> +                        size1 = (src[0] & 3);
> +                        src++;
> +                        size2 = 0;
> +                    } else {
> +                        size1 = (((src[0] & 31) + 1) << 2);
> +                        src++;
> +                        size2 = 0;
> +                    }

2 of these statements can be factored out


> +                } else {  // 110
> +                    if (src+4>src_end)
> +                        break;
> +                    size1 = (src[0] & 3);
> +                    offset = -(((src[0] & 0x10) << 12) + (src[1] << 8) + src[2]) - 1;
> +                    size2 = ((src[0] & 0xC) << 6) + src[3] + 5;
> +                    src += 4;
> +                }
> +            } else {  // 10
> +                if (src+3>src_end)
> +                    break;
> +                size1 = ( ( src[1] & 0xC0) >> 6 );
> +                offset = -(((src[1] & 0x3F) << 8) + src[2]) - 1;
> +                size2 = (src[0] & 0x3F) + 4;
> +                src += 3;
> +            }
> +        } else {  // 0
> +            if (src+2>src_end)
> +                break;

> +            size1 = (src[0] & 3);

this occurs multiple times and can be factored out as well
and it does not appear that the src+X>src_end checks are needed except
the one below


> +            offset = -( ((src[0] & 0x60) << 3) + src[1] ) - 1;
> +            size2 = ((src[0] & 0x1C) >> 2) + 3;
> +            src += 2;
> +        }
> +
> +        /* fetch strip from src */
> +        if (src+size1>src_end)
> +            break;
> +        for (i=0; i<size1; i++)
> +            dst[ CALC_LOCATION(o+i,width,stride) ] = src[i];
> +        size -= size1;
> +        src += size1;
> +        o += size1;
> +
> +        /* fetch strip within */
> +        if (size2) {
> +            /* offset is guarded by size2 */
> +            for (i=0; i<size2; i++)
> +                dst[ CALC_LOCATION(o+i,width,stride) ] = dst[ CALC_LOCATION(o+offset+i,width,stride) ];
> +            size -= size2;
> +            o += size2;
> +        }

your code is exploitable it does not contain any checks for dst size
at all
also the divide and modulo in the innermost loop are very ugly and
slow


> +    }
> +
> +    return 0;
> +}
> +
> +/**
> + * Decode inter-frame
> + * @return 0 on success, -1 on critical buffer underflow
> + */
> +static int tgv_decode_inter(TgvContext * s, const uint8_t *buf, const uint8_t *buf_end){
> +    int num_mvs;
> +    int num_blocks_raw;
> +    int num_blocks_packed;
> +    int num_blocks;
> +    int vector_bits;
> +    int i,j,x,y;
> +    GetBitContext gb;
> +    int mvbits;
> +
> +    if(buf+12>buf_end)
> +        return -1;
> +

> +    num_mvs = AV_RL16(&buf[0]);
> +    num_blocks_raw = AV_RL16(&buf[2]);
> +    num_blocks_packed = AV_RL16(&buf[4]);
> +    num_blocks = num_blocks_raw + num_blocks_packed;
> +    vector_bits = AV_RL16(&buf[6]);

vertical align

> +    buf += 12;
> +

> +    /* allocate codebook buffers as neccessary */
> +    if (s->mv_codebook==NULL) {
> +        s->mv_codebook = av_malloc(num_mvs *2*sizeof(int));
> +        s->max_num_mvs = num_mvs;
> +    }else if (num_mvs > s->max_num_mvs) {
> +        s->mv_codebook = av_realloc(s->mv_codebook, num_mvs*2*sizeof(int));
> +        s->max_num_mvs = num_mvs;
> +    }
> +
> +    if (s->block_codebook==NULL) {
> +        s->block_codebook = av_malloc(num_blocks*16*sizeof(unsigned char));
> +        s->max_num_blocks = num_blocks;
> +    }else if (num_blocks > s->max_num_blocks) {
> +        s->block_codebook = av_realloc(s->block_codebook, num_blocks*16*sizeof(unsigned char));
> +        s->max_num_blocks = num_blocks;
> +    }

av_realloc() already does handle NULL



[...]
> +    /* read uncompressed blocks */
> +    for (i=0; i<num_blocks_raw; i++) {
> +        for(j=0; j<16; j++)
> +            s->block_codebook[i][j] = buf[j];
> +        buf += 16;
> +    }

memcpy() or even better access that stuff from buf if possible


[...]
> +    if(chunk_type==kVGT_TAG) {
> +        s->frame.key_frame = 1;
> +        s->frame.pict_type = FF_I_TYPE;
> +        if (unpack(buf, buf_end, s->frame.data[0], s->avctx->width, s->frame.linesize[0])<0) {
> +            av_log(avctx, AV_LOG_WARNING, "truncated inter frame\n");
> +            return -1;
> +        }
> +    }else{

> +        s->frame.key_frame = 1;
> +        s->frame.pict_type = FF_I_TYPE;

This doesnt look correct

[...]
-- 
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/20080710/dd514c23/attachment.pgp>