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

Stefano Sabatini stefano.sabatini-lala
Sun Jan 9 02:28:17 CET 2011

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 tm2readdeltas(TM2Context *ctx, int > streamid) { > > return 0; > > } > > > > -static int tm2readstream(TM2Context *ctx, const uint8t *buf, int > streamid) { > > +static int tm2readstream(TM2Context *ctx, const uint8t *buf, int > streamid, int bufsize) { > > Nit++: "{" on following line per K&R style > > > int i; > > int cur = 0; > > int skip = 0; > > @@ -274,6 +274,11 @@ static int tm2readstream(TM2Context *ctx, const > uint8t *buf, int streamid) { > > if(len == 0) > > return 4; > > > > + if(len >= INTMAX/4-1 || len < 0 || len > bufsize) { > > nit: if( > > > + avlog(ctx->avctx, AVLOGERROR, "Error, invalid stream > size.\n"); > > + return -1; > > Please return meaningful error codes, AVERRORINVALIDDATA in this case. > > > + } > > + > > toks = AVRB32(buf); buf += 4; cur += 4; > > if(toks & 1) { > > len = AVRB32(buf); buf += 4; cur += 4; > > @@ -313,8 +318,13 @@ static int tm2readstream(TM2Context *ctx, const > uint8t *buf, int streamid) { > > len = AVRB32(buf); buf += 4; cur += 4; > > if(len > 0) { > > initgetbits(&ctx->gb, buf, (skip - cur) * 8); > > - for(i = 0; i < toks; i++)_ _> > + for(i = 0; i < toks; i++) {_ _> > + if (getbitsleft(&ctx->gb) <= 0) {_ _> > + avlog(ctx->avctx, AVLOGERROR, "Incorrect number of > tokens: %i\n", toks); > > + return -1; > > ditto

tm2readstream is called from decodeframe, which will error out in the case of -1. If -1 is not returned, decodeframe 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

More information about the ffmpeg-devel mailing list