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

[Stefano Sabatini]

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.
> 
> -- 
> 
> 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 unsigned length = value + 1;
> +            if (length > dst_size || length > buf_end - buf) // check if decompressed data overflows
> +                return AVERROR_INVALIDDATA;
> +            memcpy(dst, buf, length);
> +            dst += length;
>              buf += length;
> +            dst_size -= length;
>          } else if (value > -128) {
> -            length = -value + 1;
> -            memset(dst + x, *buf++, FFMIN(length, dst_size - x));
> -        } else { // noop
> -            continue;
> +            const unsigned length = -value + 1;
> +            if (length > dst_size || buf == buf_end) // check if decompressed data overflows
> +                return AVERROR_INVALIDDATA;
> +            memset(dst, *buf++, length);
> +            dst += length;
> +            dst_size -= length;
> +        } else if (buf == buf_end) { // noop, break loop if we reach EOL of input buffer
> +            break;
>          }
> -        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;
>                      decodeplane8(row, s->planebuf, s->planesize, plane);
>                  }
>              }
> @@ -328,7 +339,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 << 2);
>                  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;
>                      decodeplane32((uint32_t *) row, s->planebuf, s->planesize, plane);
>                  }
>              }
> @@ -336,7 +351,11 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>      } else {
>          for(y = 0; y < avctx->height ; y++ ) {
>              uint8_t *row = &s->frame.data[0][y*s->frame.linesize[0]];
> -            buf += decode_byterun(row, avctx->width, buf, buf_end);
> +            if ((err = decode_byterun(row, avctx->width, buf, buf_end)) < 0) {
> +                av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream truncated\n");
> +                return err;
> +            }
> +            buf += err;
>          }
>      }

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.

Regards.
-- 
FFmpeg = Forgiving and Fucking Miracolous Philosofic Enigmatic Geek