[FFmpeg-devel] [PATCH] Add some buffer sanity checks to indeo3

[Alex Converse]

On Tue, Feb 17, 2009 at 4:05 PM, Michael Niedermayer <michaelni at gmx.at>wrote:

> On Mon, Feb 16, 2009 at 02:13:02PM -0500, Alex Converse wrote:
> > This helps with but doesn't completely fix 1-dog.avi
> >
> > Regards,
> > Alex Converse
>
> > diff --git a/libavcodec/indeo3.c b/libavcodec/indeo3.c
> > index 6173c6f..3663d40 100644
> > --- a/libavcodec/indeo3.c
> > +++ b/libavcodec/indeo3.c
> > @@ -975,7 +975,7 @@ static av_cold int indeo3_decode_init(AVCodecContext
> *avctx)
> >      return ret;
> >  }
> >
> > -static unsigned long iv_decode_frame(Indeo3DecodeContext *s,
> > +static int iv_decode_frame(Indeo3DecodeContext *s,
> >                                       const uint8_t *buf, int buf_size)
> >  {
> >      unsigned int image_width, image_height,
>
> > @@ -1006,6 +1006,11 @@ static unsigned long
> iv_decode_frame(Indeo3DecodeContext *s,
> >      hdr_pos = buf_pos;
> >      if(data_size == 0x80) return 4;
> >
> > +    if(y_offset >= buf_size - 16 || v_offset >= buf_size - 16 ||
> u_offset >= buf_size -16) {
> > +        av_log(s->avctx, AV_LOG_ERROR, "y/u/v offset outside buffer\n");
> > +        return -1;
> > +    }
> > +
> >      if(flags & 0x200) {
> >          s->cur_frame = s->iv_frame + 1;
> >          s->ref_frame = s->iv_frame;
>
> FFMAX3(y_offset, u_offset, v_offset) >= buf_size - 16
>
>
>
> > @@ -1016,6 +1021,10 @@ static unsigned long
> iv_decode_frame(Indeo3DecodeContext *s,
> >
> >      buf_pos = buf + 16 + y_offset;
> >      mc_vector_count = bytestream_get_le32(&buf_pos);
> > +    if(16 + y_offset + 2*mc_vector_count >= buf_size) {
> > +        av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too large\n");
> > +        return -1;
> > +    }
> >
> >      iv_Decode_Chunk(s, s->cur_frame->Ybuf, s->ref_frame->Ybuf,
> image_width,
> >                      image_height, buf_pos + mc_vector_count * 2,
> cb_offset, hdr_pos, buf_pos,
>
> can overflow
>
>
> > @@ -1026,6 +1035,10 @@ static unsigned long
> iv_decode_frame(Indeo3DecodeContext *s,
> >
> >          buf_pos = buf + 16 + v_offset;
> >          mc_vector_count = bytestream_get_le32(&buf_pos);
> > +        if(16 + v_offset + 2*mc_vector_count >= buf_size) {
> > +            av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too
> large\n");
> > +            return -1;
> > +        }
> >
> >          iv_Decode_Chunk(s, s->cur_frame->Vbuf, s->ref_frame->Vbuf,
> chroma_width,
> >                  chroma_height, buf_pos + mc_vector_count * 2, cb_offset,
> hdr_pos, buf_pos,
> > @@ -1033,6 +1046,10 @@ static unsigned long
> iv_decode_frame(Indeo3DecodeContext *s,
> >
> >          buf_pos = buf + 16 + u_offset;
> >          mc_vector_count = bytestream_get_le32(&buf_pos);
> > +        if(16 + u_offset + 2*mc_vector_count >= buf_size) {
> > +            av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too
> large\n");
> > +            return -1;
> > +        }
> >
> >          iv_Decode_Chunk(s, s->cur_frame->Ubuf, s->ref_frame->Ubuf,
> chroma_width,
> >                  chroma_height, buf_pos + mc_vector_count * 2, cb_offset,
> hdr_pos, buf_pos,
>
> so can these


I'm aware iv_Decode_Chunk() can still overflow. It's a big monolithic
function with no documentation. I didn't have the time nor the energy to try
to unwind and make sense of it. Still these checks lessen the exposure of
the unsafe code to bad input.

--Alex