[FFmpeg-devel] [PATCH] IFF PBM/ILBM decoder

[Michael Niedermayer]

On Mon, Feb 01, 2010 at 11:33:30PM +1100, pross at xvid.org wrote:
> On Thu, Jan 28, 2010 at 09:06:43PM +0100, Michael Niedermayer wrote:
> > On Wed, Jan 27, 2010 at 10:46:14PM +1100, pross at xvid.org wrote:
> > > Hi, the war against Electronic Arts formats continues.
> > > 
> > > samples: /Mplayer/incoming/lbm
> > >          http://samples.mplayerhq.hu/image-samples/ASH.LBM
> > > 
> > > Some identing cleanup is required after applying this patch.
> > > 
> > > -- Peter
> > > (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
> > 
> > [..]
> > [...]
> 
> > > +    for(y = 0; y < avctx->height ; y++ ) {
> > > +        uint8_t *row = &frame->data[0][ y*frame->linesize[0] ];
> > > +        if (avctx->codec_tag == MKTAG('I','L','B','M')) { //interleaved
> > > +            memset(row, 0, avctx->width);
> > > +            for (plane = 0; plane < avctx->bits_per_coded_sample; plane++) {
> > > +                for(x = 0; x < planewidth && buf < buf_end; ) {
> > > +                    char value = *buf++;
> > > +                    int length;
> > > +                    if (value >= 0) {
> > > +                        length = value + 1;
> > > +                        imemcpy(row, buf, x, avctx->bits_per_coded_sample, plane, FFMIN(length, buf_end-buf));
> > > +                        buf += length;
> > > +                    } else if (value > -128) {
> > > +                        length = -value + 1;
> > > +                        imemset(row, *buf++, x, avctx->bits_per_coded_sample, plane, length);
> > 
> > these look like they are missing output buffer size checks
> 
> Fixed.
> 
> -- Peter
> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

[...]
> +/**
> + * Convert CMAP buffer (stored in extradata) to lavc palette format
> + */
> +int ff_cmap_read_palette(AVCodecContext *avctx, uint32_t *pal)
> +{
> +    int i;
> +
> +    int count = 2 << (avctx->bits_per_coded_sample - 1);

2 << -1 ?
why not
1<< avctx->bits_per_coded_sample
?


> +    if (avctx->extradata_size < count * 3) {
> +        av_log(avctx, AV_LOG_ERROR, "palette data underflow\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +    for (i=0; i < count; i++) {
> +        pal[i] = AV_RB24( avctx->extradata + i*3 );
> +    }

what if count is bigger than 256 ?

also maybe testing the decoder & demuxer with a fuzzer would be a
good idea


[...]
> +static int decode_frame_ilbm(AVCodecContext *avctx,
> +                            void *data, int *data_size,
> +                            AVPacket *avpkt)
> +{
> +    AVFrame *frame = avctx->priv_data;
> +    const uint8_t *buf = avpkt->data;
> +    int buf_size = avpkt->size;
> +    int planewidth = avctx->width / avctx->bits_per_coded_sample;
> +    int y, plane;
> +
> +    if (avctx->reget_buffer(avctx, frame) < 0){
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    if (buf_size < avctx->width * avctx->height) {
> +        av_log(avctx, AV_LOG_ERROR, "buffer underflow\n");
> +        return -1;
> +    }
> +
> +    for(y = 0; y < avctx->height; y++ ) {
> +        uint8_t *row = &frame->data[0][ y*frame->linesize[0] ];
> +        memset(row, 0, avctx->width);
> +        for (plane = 0; plane < avctx->bits_per_coded_sample; plane++) {
> +            imemcpy(row, buf, 0, avctx->bits_per_coded_sample, plane, planewidth);
> +            buf += planewidth;
> +        }

can this write outside the array if bits_per_coded_sample is > 8 ?

except these id say your patch is ok

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20100202/ce3d0d5f/attachment.pgp>