| Message ID | 1ab7c3994301a243fc64d59d6a08e3a2b364e411.1666774269.git.pross@xvid.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/eatgq: prevent out of bounds memory access and endless loop | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Peter Ross: > --- > libavcodec/eatgq.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/eatgq.c b/libavcodec/eatgq.c > index 89e9f20880..fdda8286ef 100644 > --- a/libavcodec/eatgq.c > +++ b/libavcodec/eatgq.c > @@ -56,7 +56,7 @@ static av_cold int tgq_decode_init(AVCodecContext *avctx) > return 0; > } > > -static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb) > +static int tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb) > { > const uint8_t *scantable = ff_zigzag_direct; > int i, j, value; > @@ -73,7 +73,9 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb > case 1: > skip_bits(gb, 2); > value = get_bits(gb, 6); > - for (j = 0; j < value; j++) > + if (!value) > + return AVERROR_INVALIDDATA; > + for (j = 0; j < value && i < 64; j++) > block[scantable[i++]] = 0; > break; > case 6: > @@ -100,6 +102,7 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb > } > } > block[0] += 128 << 4; > + return 0; > } > > static void tgq_idct_put_mb(TgqContext *s, int16_t (*block)[64], AVFrame *frame, > @@ -161,7 +164,8 @@ static int tgq_decode_mb(TgqContext *s, GetByteContext *gbyte, > return ret; > > for (i = 0; i < 6; i++) > - tgq_decode_block(s, s->block[i], &gb); > + if ((ret = tgq_decode_block(s, s->block[i], &gb)) < 0) > + return ret; > tgq_idct_put_mb(s, s->block, frame, mb_x, mb_y); > bytestream2_skip(gbyte, mode); > } else { > > The '4' case can also overread. But actually I don't like the idea of adding further checks into the main loop; mind if I send an alternative solution? - Andreas
On Wed, Oct 26, 2022 at 01:41:57PM +0200, Andreas Rheinhardt wrote: > Peter Ross: > > --- > > libavcodec/eatgq.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/eatgq.c b/libavcodec/eatgq.c > > index 89e9f20880..fdda8286ef 100644 > > --- a/libavcodec/eatgq.c > > +++ b/libavcodec/eatgq.c > > @@ -56,7 +56,7 @@ static av_cold int tgq_decode_init(AVCodecContext *avctx) > > return 0; > > } > > > > -static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb) > > +static int tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb) > > { > > const uint8_t *scantable = ff_zigzag_direct; > > int i, j, value; > > @@ -73,7 +73,9 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb > > case 1: > > skip_bits(gb, 2); > > value = get_bits(gb, 6); > > - for (j = 0; j < value; j++) > > + if (!value) > > + return AVERROR_INVALIDDATA; > > + for (j = 0; j < value && i < 64; j++) > > block[scantable[i++]] = 0; > > break; > > case 6: > > @@ -100,6 +102,7 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb > > } > > } > > block[0] += 128 << 4; > > + return 0; > > } > > > > static void tgq_idct_put_mb(TgqContext *s, int16_t (*block)[64], AVFrame *frame, > > @@ -161,7 +164,8 @@ static int tgq_decode_mb(TgqContext *s, GetByteContext *gbyte, > > return ret; > > > > for (i = 0; i < 6; i++) > > - tgq_decode_block(s, s->block[i], &gb); > > + if ((ret = tgq_decode_block(s, s->block[i], &gb)) < 0) > > + return ret; > > tgq_idct_put_mb(s, s->block, frame, mb_x, mb_y); > > bytestream2_skip(gbyte, mode); > > } else { > > > > > > The '4' case can also overread. But actually I don't like the idea of > adding further checks into the main loop; mind if I send an alternative > solution? please go for it. -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
diff --git a/libavcodec/eatgq.c b/libavcodec/eatgq.c index 89e9f20880..fdda8286ef 100644 --- a/libavcodec/eatgq.c +++ b/libavcodec/eatgq.c @@ -56,7 +56,7 @@ static av_cold int tgq_decode_init(AVCodecContext *avctx) return 0; } -static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb) +static int tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb) { const uint8_t *scantable = ff_zigzag_direct; int i, j, value; @@ -73,7 +73,9 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb case 1: skip_bits(gb, 2); value = get_bits(gb, 6); - for (j = 0; j < value; j++) + if (!value) + return AVERROR_INVALIDDATA; + for (j = 0; j < value && i < 64; j++) block[scantable[i++]] = 0; break; case 6: @@ -100,6 +102,7 @@ static void tgq_decode_block(TgqContext *s, int16_t block[64], GetBitContext *gb } } block[0] += 128 << 4; + return 0; } static void tgq_idct_put_mb(TgqContext *s, int16_t (*block)[64], AVFrame *frame, @@ -161,7 +164,8 @@ static int tgq_decode_mb(TgqContext *s, GetByteContext *gbyte, return ret; for (i = 0; i < 6; i++) - tgq_decode_block(s, s->block[i], &gb); + if ((ret = tgq_decode_block(s, s->block[i], &gb)) < 0) + return ret; tgq_idct_put_mb(s, s->block, frame, mb_x, mb_y); bytestream2_skip(gbyte, mode); } else {