[Ffmpeg-devel] [PATCH] ported SGI decoder to the new API

[Michael Niedermayer]

Hi

On Wed, Apr 04, 2007 at 12:11:08AM +0800, Xiaohui Sun wrote:
> Hi,
>   I updated the patch.
[...]
> 
> >
> >> +
> >> +    while (1) {
> >
> >> +        if(in_buf + 1 > end_buf) return -1;
> >
> >this check isnt really needed as the input array is guranteed to be 8 
> >bytes
> >larger then end_buf
> >
> removed

you removed the wrong check, now it can read >100 bytes too much, there
is not that much extra padding at the end, though over reading is far
less serious than overwriting


> 
> >
> >[...]
> >> +            start_offset = bytestream_get_be32(&start_table);
> >> +            if(start_offset > end_buf - in_buf) {
> >> +                return AVERROR_INVALIDDATA;
> >> +            }
> >
> >excelent, finally the check looks good
> >
> >
> >> +            if (expand_rle_row(in_buf + start_offset, end_buf, 
> >dest_row,
> >> +                dest_row + s->linesize, z, s->depth) != s->width)
> >> +                return AVERROR_INVALIDDATA;
> >
> >linesize can be negative
> >
> I modified, but not sure.

it should have been dest_row + width*pixelsize or dest_row + FFABS(s->linesize)


[...]
> +/**
> + * expand an RLE row into a channel
> + * @param in_buf input buffer
> + * @param out_ptr output buffer
> + * @param end_ptr end of line in output buffer
> + * @param pixelstride pixel stride of input buffer
> + * @return Size of output in bytes, -1 if buffer overflows
> + */
> +static int expand_rle_row(uint8_t *in_buf, uint8_t* end_buf,
> +            unsigned char *out_ptr, uint8_t* end_ptr, int pixelstride)
> +{
> +    unsigned char pixel, count;
> +    unsigned char *orig = out_ptr;
> +
> +    while (1) {
> +        if(in_buf + 1 > end_buf) return -1;
> +        pixel = bytestream_get_byte(&in_buf);
> +        if (!(count = (pixel & 0x7f))) {
> +            return (out_ptr - orig) / pixelstride;
> +        }
> +
> +        /* check for buffer overflow */
> +        if(out_ptr + pixelstride * count >= end_ptr) return -1;

please rename end_ptr to out_end and end_buf to in_end


[...]
> +    out_ptr = p->data[0];
> +
> +    end_ptr = out_ptr + p->linesize[0] * s->height;
> +
> +    if(p->linesize[0] < 0) {
> +        s->linesize = - p->linesize[0];
> +        FFSWAP(uint8_t*, end_ptr, out_ptr);
> +    } else {
> +        s->linesize = p->linesize[0];
> +    }

this is wrong
first, it simply wont work as the pointers are off by one line
second, if the pointers where not off by one line it would store a
vertically fliped image


[...]
> +    avcodec_get_frame_defaults((AVFrame*)&s->picture);
> +    avctx->coded_frame = (AVFrame*)&s->picture;

unneeded casts


[...]
> +    .pix_fmts= (enum PixelFormat[]){PIX_FMT_RGB24, PIX_FMT_RGB32, PIX_FMT_PAL8, PIX_FMT_GRAY8, -1},

PIX_FMT_RGB32 is wrong, i think this was already correct in a previous
patch ...

PIX_FMT_RGB32 store native endian 32bit integers with rgb in them so
it differs on little and big endian, the bytewise readeing and writing
code in your codec though writes the same format on both little and
big endian so PIX_FMT_RGB32 cannot be correct

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20070404/675109da/attachment.pgp>