[Ffmpeg-devel] [PATCH] DXA demuxer and decoder

Michael Niedermayer michaelni
Tue Mar 13 23:39:52 CET 2007


Hi

On Tue, Mar 13, 2007 at 08:27:23PM +0200, Kostya wrote:
[...]

[configure/makefile stuff i dont maintain ...]

[...]
> +        c->pic.key_frame = !(compr & 1);
> +        c->pic.pict_type = (compr & 1) ? FF_P_TYPE : FF_I_TYPE;
> +        if(compr & 1){
> +            for(j = 0; j < avctx->height; j++){
> +                for(i = 0; i < avctx->width; i++)
> +                    outptr[i] ^= srcptr[i];
> +                outptr += stride;
> +                srcptr += avctx->width;
> +            }
> +        }else{
> +            for(i = 0; i < avctx->height; i++){
> +                memcpy(outptr, srcptr, avctx->width);
> +                outptr += stride;
> +                srcptr += avctx->width;
> +            }
> +        }

for(j = 0; j < avctx->height; j++){
    if(compr & 1){
        for(i = 0; i < avctx->width; i++)
            outptr[i] ^= srcptr[i];
    }else
        memcpy(outptr, srcptr, avctx->width);
    outptr += stride;
    srcptr += avctx->width;
}



[...]
> +    *data_size = sizeof(AVFrame);
> +    *(AVFrame*)data = c->pic;
> +
> +    avctx->release_buffer(avctx, &c->prev);
> +    c->prev = c->pic;

please use FFSWAP, instead of duplicating the AVFrame, ive a bad feeling
about this, it has the potential for memleaks and other weirdness
this also makes c->pic.data[0] = NULL; unneeded


[...]
> +    c->pic.data[0] = NULL;

please remove this, if its not NULL thats a bug, the NULL check is a feature
to detect messup with allocating and deallocating avframes ...


[...]
> +            if(av_new_packet(pkt, 4 + pal_size) < 0)
> +                return AVERROR_NOMEM;

> +            pkt->size = 4 + pal_size;

hmm what does that do?


> +            pkt->stream_index = c->has_sound;

if you create the mandatory video stream first then video where always 0
and audio if it exists always 1


[...]
> +            get_buffer(&s->pb, buf + 4, DXA_EXTRA_SIZE - 4);
> +            size = AV_RB32(buf + 5);
> +            if(av_new_packet(pkt, size + DXA_EXTRA_SIZE + pal_size) < 0)
> +                return AVERROR_NOMEM;
> +            memcpy(pkt->data + pal_size, buf, DXA_EXTRA_SIZE);
> +            ret = get_buffer(&s->pb, pkt->data + DXA_EXTRA_SIZE + pal_size, size);

integer overflow leading to writing data over the end of an array
probably not exploitable but still

more precissely
size is large
size + DXA_EXTRA_SIZE + pal_size overflows and av_new_packet() is called with
a small but valid size
the following memcpy writes over the end of the array though the data which
is written can not entirely freely be selected by the attacker
following get_buffer() will detect the too large size and fail


except these patch ok

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

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070313/f86f7810/attachment.pgp>



More information about the ffmpeg-devel mailing list