| Message ID | 20180628225745.14411-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | d08d4a8c7387e758d439b0592782e4cfa2b4d6a4 |
| Headers | show |
> Fixes: out of array access > Fixes: ffmpeg_bof_4.avi > Fixes: ffmpeg_bof_5.avi > Fixes: ffmpeg_bof_6.avi > > Found-by: Thuan Pham, Marcel Böhme, Andrew Santosa and Alexandru Razvan Caciulescu with AFLSmart > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc> > --- > libavcodec/vc1_block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > index 5427de4ec6..74d5e27803 100644 > --- a/libavcodec/vc1_block.c > +++ b/libavcodec/vc1_block.c > @@ -204,7 +204,7 @@ static void vc1_put_blocks_clamped(VC1Context *v, int put_signed) > if ((edges&8) && \ > s->mb_y == ((s->mb_height >> v->field_mode) - 1)) \ > mquant = -v->altpq; \ > - if (!mquant || mquant > 31) { \ > + if (!mquant || mquant > 31 || mquant < -31) { \ > av_log(v->s.avctx, AV_LOG_ERROR, \ > "Overriding invalid mquant %d\n", mquant); \ > mquant = 1; \ > -- > 2.18.0 LGTM However, we could consider to use saturation for invalid mquant values. Something like: mquant = mquant ? av_clip(mquant, -31, 31) : 1; I would prefer to catch illegal values at the earliest occasion. Illegal v->pq or v->altpq should be catched earlier, in my view. A the current implementation is technically correct, this can wait for another time. Regards, Jerome
On Fri, Jun 29, 2018 at 12:01:42PM +0200, Jerome Borsboom wrote: > > Fixes: out of array access > > Fixes: ffmpeg_bof_4.avi > > Fixes: ffmpeg_bof_5.avi > > Fixes: ffmpeg_bof_6.avi > > > > Found-by: Thuan Pham, Marcel Böhme, Andrew Santosa and Alexandru Razvan Caciulescu with AFLSmart > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc> > > --- > > libavcodec/vc1_block.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c > > index 5427de4ec6..74d5e27803 100644 > > --- a/libavcodec/vc1_block.c > > +++ b/libavcodec/vc1_block.c > > @@ -204,7 +204,7 @@ static void vc1_put_blocks_clamped(VC1Context *v, int put_signed) > > if ((edges&8) && \ > > s->mb_y == ((s->mb_height >> v->field_mode) - 1)) \ > > mquant = -v->altpq; \ > > - if (!mquant || mquant > 31) { \ > > + if (!mquant || mquant > 31 || mquant < -31) { \ > > av_log(v->s.avctx, AV_LOG_ERROR, \ > > "Overriding invalid mquant %d\n", mquant); \ > > mquant = 1; \ > > -- > > 2.18.0 > > LGTM will apply > > However, we could consider to use saturation for invalid mquant values. > > Something like: > > mquant = mquant ? av_clip(mquant, -31, 31) : 1; yes, it probably could be slightly better in some rare cases where the decoder isnt totally confused and only produces random giberish anymore > > > I would prefer to catch illegal values at the earliest occasion. Illegal > v->pq or v->altpq should be catched earlier, in my view. A the current > implementation is technically correct, this can wait for another time. yes, i agree thx [...]
diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c index 5427de4ec6..74d5e27803 100644 --- a/libavcodec/vc1_block.c +++ b/libavcodec/vc1_block.c @@ -204,7 +204,7 @@ static void vc1_put_blocks_clamped(VC1Context *v, int put_signed) if ((edges&8) && \ s->mb_y == ((s->mb_height >> v->field_mode) - 1)) \ mquant = -v->altpq; \ - if (!mquant || mquant > 31) { \ + if (!mquant || mquant > 31 || mquant < -31) { \ av_log(v->s.avctx, AV_LOG_ERROR, \ "Overriding invalid mquant %d\n", mquant); \ mquant = 1; \
Fixes: out of array access Fixes: ffmpeg_bof_4.avi Fixes: ffmpeg_bof_5.avi Fixes: ffmpeg_bof_6.avi Found-by: Thuan Pham, Marcel Böhme, Andrew Santosa and Alexandru Razvan Caciulescu with AFLSmart Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vc1_block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)