[FFmpeg-devel] [PATCH] add sanity checks to truemotion2 (issue 2512)

[Stefano Sabatini]

On date Saturday 2011-01-08 19:34:41 -0500, Daniel Kang encoded:
> On Sat, Jan 8, 2011 at 7:23 PM, Stefano Sabatini <
> stefano.sabatini-lala at poste.it> wrote:
> 
> > On date Saturday 2011-01-08 19:10:06 -0500, Daniel Kang encoded:
> > > truemotion2 videos with invalid metadata crashes ffmpeg. There are
> > > multiple places where the decoder (demuxer?) overreads a buffer. The
> > > patch adds several checks for this. Are there any comments?
> >
> > > From fb6f2b4591c059887fa32c5277aade5964b6bc70 Mon Sep 17 00:00:00 2001
> > > From: Daniel Kang <daniel.d.kang at gmail.com>
> > > Date: Sat, 8 Jan 2011 16:11:31 -0500
> > > Subject: [PATCH] Fix several errors in truemotion2 decoding
> > >
> > > ---
> > >  libavcodec/truemotion2.c |   16 +++++++++++++---
> > >  1 files changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > > index 5013a9e..9c08d61 100644
> > > --- a/libavcodec/truemotion2.c
> > > +++ b/libavcodec/truemotion2.c
> > > @@ -260,7 +260,7 @@ static int tm2_read_deltas(TM2Context *ctx, int
> > stream_id) {
> > >      return 0;
> > >  }
> > >
> > > -static int tm2_read_stream(TM2Context *ctx, const uint8_t *buf, int
> > stream_id) {
> > > +static int tm2_read_stream(TM2Context *ctx, const uint8_t *buf, int
> > stream_id, int buf_size) {
> >
> > Nit++: "{" on following line per K&R style
> >
> > >      int i;
> > >      int cur = 0;
> > >      int skip = 0;
> > > @@ -274,6 +274,11 @@ static int tm2_read_stream(TM2Context *ctx, const
> > uint8_t *buf, int stream_id) {
> > >      if(len == 0)
> > >          return 4;
> > >
> > > +    if(len >= INT_MAX/4-1 || len < 0 || len > buf_size) {
> >
> > nit: if_(
> >
> > > +        av_log(ctx->avctx, AV_LOG_ERROR, "Error, invalid stream
> > size.\n");
> > > +        return -1;
> >
> > Please return meaningful error codes, AVERROR_INVALIDDATA in this case.
> >
> > > +    }
> > > +
> > >      toks = AV_RB32(buf); buf += 4; cur += 4;
> > >      if(toks & 1) {
> > >          len = AV_RB32(buf); buf += 4; cur += 4;
> > > @@ -313,8 +318,13 @@ static int tm2_read_stream(TM2Context *ctx, const
> > uint8_t *buf, int stream_id) {
> > >      len = AV_RB32(buf); buf += 4; cur += 4;
> > >      if(len > 0) {
> > >          init_get_bits(&ctx->gb, buf, (skip - cur) * 8);
> > > -        for(i = 0; i < toks; i++)
> > > +        for(i = 0; i < toks; i++) {
> > > +            if (get_bits_left(&ctx->gb) <= 0) {
> > > +                av_log(ctx->avctx, AV_LOG_ERROR, "Incorrect number of
> > tokens: %i\n", toks);
> > > +                return -1;
> >
> > ditto
> 
> 
> tm2_read_stream is called from decode_frame, which will error out in the
> case of -1. If -1 is not returned, decode_frame will continue to decode.
> For values that are not -1, t is added to skip, which could
> (potentially) decrementing it, which I do not believe is the correct
> behavior.

That should be changed as well, but not as part of this patch.

> I have updated the patch with the other comments.

Thanks.
-- 
FFmpeg = Fierce and Freak Mega Pitiless Entertaining Gigant