[FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction

[Andreas Rheinhardt]

Zhao Zhili:
> From: Zhao Zhili <zhilizhao at tencent.com>
> 
> Fixes ticket #9816.
> Regression since ed0001482a74b60f3d5bc5.
> 
> Prediction value larger than INT32_MAX should be treated as
> negative. The code already depends on undefined right shift
> behavior before the patch, which doesn't get fixed by the patch.
> 
> Signed-off-by: Zhao Zhili <zhilizhao at tencent.com>
> ---
>  libavcodec/apedec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> index a7c38bce1b..5f2af2e147 100644
> --- a/libavcodec/apedec.c
> +++ b/libavcodec/apedec.c
> @@ -1198,7 +1198,7 @@ static av_always_inline int predictor_update_filter(APEPredictor64 *p,
>                    p->buf[delayB - 3] * p->coeffsB[filter][3] +
>                    p->buf[delayB - 4] * p->coeffsB[filter][4];
>  
> -    p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
> +    p->lastA[filter] = decoded + ((int32_t)((uint64_t)predictionA + (predictionB >> 1)) >> 10);
>      p->filterA[filter] = p->lastA[filter] + ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
>  
>      sign = APESIGN(decoded);

1. Right shifts of negative numbers are implementation-defined, not
undefined. And we rely on the implementation to do the sane thing for
two's complement, so it is defined for us.
2. You are not necessarily treating them as negative, you simply discard
the upper 32 bits and the result could be positive. And you discard the
upper bits before you discard the lowest 10 bits, so the result has
effectively only 22 bits. Is this really intended? If it is, you could
perform the addition predictionA + (predictionB >> 1) in 32 bits.
3. I see one possibility for UB in this: The outermost addition on the
right side is a signed addition, so it could overflow.

- Andreas