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

[Stefano Sabatini]

On date Saturday 2011-01-08 20:36:15 -0500, Daniel Kang encoded:
> On Sat, Jan 8, 2011 at 8:28 PM, Stefano Sabatini <
> stefano.sabatini-lala at poste.it> wrote:
> 
> > 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.
> 
> 
>  Is this patch okay with you now?

Yes.
 
> How should the return values issue be dealt with?

I suppose by fixing the check => if (err < 0) rather than if (err ==
-1), but this isn't related with your patch.
-- 
FFmpeg = Fanciful and Faithless Mystic Perennial Elegant Game