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

[Daniel Kang]

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.

I have updated the patch with the other comments.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: truemotion2_checks.diff
Type: application/octet-stream
Size: 2307 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110108/8e44f541/attachment.obj>