[FFmpeg-devel] [PATCH] avcodec/vc1_block: Fix mqaunt check for negative values

[Jerome Borsboom]

> 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