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

[Michael Niedermayer]

On Sun, May 23, 2010 at 05:31:46PM +0200, Sebastian Vater wrote:
> Ronald S. Bultje a ?crit :
> > Hi,
> >
> > On Sun, May 16, 2010 at 6:35 PM, Sebastian Vater
> > <cdgs.basty at googlemail.com> wrote:
> >   
> >> Stefano Sabatini a ?crit :
> >>     
> >>> On date Monday 2010-05-17 00:08:55 +0200, Sebastian Vater encoded:
> >>>
> >>>       
> >>>> @@ -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.
> >>>
> >>>       
> >> Fixed by changing err(or) to res(ult).
> >>     
> > [..]
> >   
> >> @@ -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 ((res = decode_byterun(row, avctx->width, buf, buf_end)) < 0) {
> >> +                av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream truncated\n");
> >> +                return res;
> >> +            }
> >> +            buf += res;
> >>          }
> >>      }
> >>     
> >
> >
> >   
> > And I agree with Stefano that the log msg should be in
> > decode_byterun() also, a goto here is fine with me.
> >   
> 
> Fixed. For preventing the function become non-inlined then I have added
> av_always_inline to it. I also pass AVCodecContext
> to it.
> Hope this new patch is fine.
> 
> -- 
> 
> Best regards,
>                    :-) Basty/CDGS (-:
> 

>  iff.c |   58 ++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 8519ee6c6a67610cb02eb831b3261e324bcee6ed  iff-byterun1-error.patch
> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> index bcf4929..5e4c55a 100644
> --- a/libavcodec/iff.c
> +++ b/libavcodec/iff.c
> @@ -226,27 +226,43 @@ 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) {
> +static av_always_inline int decode_byterun(AVCodecContext *const avctx,
> +                                           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) {
> +        av_log(avctx, AV_LOG_ERROR, "IFF byterun1 stream buffer overflow\n");
> +        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;

whats that cast good for?

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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20100524/7edc510f/attachment.pgp>