[FFmpeg-devel] [PATCH] IFF: Add error checking to byterun1 decoder

[Stefano Sabatini]

On date Monday 2010-05-17 00:08:55 +0200, Sebastian Vater encoded:
> Stefano Sabatini a ?crit :
> > On date Sunday 2010-05-16 22:17:02 +0200, Sebastian Vater encoded:
> >   
> >> Sebastian Vater a ?crit :
> >>     
> >>> Hello guys!
> >>>
> >>> As suggested by Ronald, I separated error checking in byterun1 decoder
> >>> from the function merge patch.
> >>>
> >>> So this just patches latest iff-function-merge.patch in order to add
> >>> error checking to byterun1 decompression routines by returning an
> >>> negative number of bytes consumed.
> >>>
> >>> Besides this, I optimized the decode_byterun1 routine a bit since it can
> >>> now assume that width is always > 0
> >>>   
> >>>       
> >> 10l to me...just forgot to add the int err declaration.
> >>
> >> Anyway, I used the chance to add an error message in case of byterun1
> >> corrupted stream, too.
> >> So please review this one.
> >>
> >>
> >>     
> > Patch looks OK to me, but I'd prefer the error message to be issued by
> > decode_byterun() rather than duplicate the error logging in the
> > calling code.
> >   
> 
> Thank you!
> 
> I just fixed some small errors for boundary checkings, the previous
> patch could read too much bytes.
> Also I "nicified" comments and code look & feel.
> 
> I have leaved the error message handling as is for now, since I either
> would have to pass NULL to av_log or to change function prototype once
> again to pass avctx to decode_byterun as well. The third option, of
> course, is to keep the error displaying outside decode_byterun, as it is
> now...
> 
> -- 
> 
> Best regards,
>                    :-) Basty/CDGS (-:
> 

> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> index bcf4929..186169e 100644
> --- a/libavcodec/iff.c
> +++ b/libavcodec/iff.c
> @@ -226,27 +226,34 @@ static void decodeplane32(uint32_t *dst, const uint8_t *buf, int buf_size, int p
>   * @param dst_size the destination plane size in bytes
>   * @param buf the source byterun1 compressed bitstream
>   * @param buf_end the EOF of source byterun1 compressed bitstream
> - * @return number of consumed bytes in byterun1 compressed bitstream
> + * @return consumed bytes in compressed bitstream or negative error code otherwise
>  */
>  static int decode_byterun(uint8_t *dst, int dst_size,
>                            const uint8_t *buf, const uint8_t *const buf_end) {
>      const uint8_t *const buf_start = buf;
> -    unsigned x;
> -    for (x = 0; x < dst_size && buf < buf_end;) {
> -        unsigned length;
> +    if (buf >= buf_end)
> +        return AVERROR_INVALIDDATA;
> +    do {
>          const int8_t value = *buf++;
>          if (value >= 0) {
> -            length = value + 1;
> -            memcpy(dst + x, buf, FFMIN3(length, dst_size - x, buf_end - buf));
> +            const int length = (unsigned) value + 1;
> +            if (length > dst_size || length > (int) (buf_end - buf)) // overflow?
> +                return AVERROR_INVALIDDATA;
> +            memcpy(dst, buf, length);
> +            dst_size -= length;
> +            dst += length;
>              buf += length;
>          } else if (value > -128) {
> -            length = -value + 1;
> -            memset(dst + x, *buf++, FFMIN(length, dst_size - x));
> -        } else { // noop
> -            continue;
> +            const int length = (unsigned) -value + 1;
> +            if (length > dst_size || buf >= buf_end) // overflow?
> +                return AVERROR_INVALIDDATA;
> +            memset(dst, *buf++, length);
> +            dst += length;
> +            dst_size -= length;
> +        } else if (buf >= buf_end) { // noop, return error if reached EOF of input
> +            return AVERROR_INVALIDDATA;
>          }
> -        x += length;
> -    }
> +    } while (dst_size > 0);
>      return buf - buf_start;
>  }
>  
> @@ -306,7 +313,7 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>      const uint8_t *buf = avpkt->data;
>      int buf_size = avpkt->size;
>      const uint8_t *buf_end = buf+buf_size;
> -    int y, plane;
> +    int y, plane, err;
>  
>      if (avctx->reget_buffer(avctx, &s->frame) < 0){
>          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> @@ -319,7 +326,11 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>                  uint8_t *row = &s->frame.data[0][ y*s->frame.linesize[0] ];
>                  memset(row, 0, avctx->width);
>                  for (plane = 0; plane < avctx->bits_per_coded_sample; plane++) {
> -                    buf += decode_byterun(s->planebuf, s->planesize, buf, buf_end);
> +                    if ((err = decode_byterun(s->planebuf, s->planesize, buf, buf_end)) < 0) {
> +                        av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream truncated\n");
> +                        return err;
> +                    }
> +                    buf += err;

buf += err looks strange, buf += ret should look saner.

I still slightly prefer having av_log(... "IFF byterun1 stream
truncated\n") in decode_byterun(), but that's really a nit so do like
you prefer.

Regards.
-- 
FFmpeg = Fostering and Fabulous Merciless Pitiless Enlightened God