[FFmpeg-devel] [PATCH 4/4] avcodec/apedec: Fix multiple integer overflows in filter_3800()

[Lynne]

Jun 16, 2019, 10:57 AM by michael at niedermayer.cc:

> Fixes: left shift of negative value -4
> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in type 'int'
> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be represented in type 'int'
> Fixes: 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavcodec/apedec.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> index 61ebfdafd5..f1bfc634c2 100644
> --- a/libavcodec/apedec.c
> +++ b/libavcodec/apedec.c
> @@ -859,22 +859,22 @@ static av_always_inline int filter_3800(APEPredictor *p,
>  return predictionA;
>  }
>  d2 =  p->buf[delayA];
> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) << 3);
> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) * 8);
>  d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>  d4 =  p->buf[delayB];
>  
> -    predictionA = d0 * p->coeffsA[filter][0] +
> -                  d1 * p->coeffsA[filter][1] +
> -                  d2 * p->coeffsA[filter][2];
> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
> +                  d1 * (unsigned)p->coeffsA[filter][1] +
> +                  d2 * (unsigned)p->coeffsA[filter][2];
>  
>  sign = APESIGN(decoded);
>  p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>  p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>  p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>  
> -    predictionB = d3 * p->coeffsB[filter][0] -
> -                  d4 * p->coeffsB[filter][1];
> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
> +                  d4 * (unsigned)p->coeffsB[filter][1];
>  p->lastA[filter] = decoded + (predictionA >> 11);
>  sign = APESIGN(p->lastA[filter]);
>  p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer, int order, int shift, int len
>  dotprod = 0;
>  sign = APESIGN(buffer[i]);
>  for (j = 0; j < order; j++) {
> -            dotprod += delay[j] * coeffs[j];
> +            dotprod += delay[j] * (unsigned)coeffs[j];
>  coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>  }
>  buffer[i] -= dotprod >> shift;
>

This code is DSP, overflows should be allowed in case input is corrupt.
This isn't even the best way to solve this, you can just make the array types unsigned.

NAK to this patchset and all future patchsets fixing overflows in DSPs and timeouts until
you explain exactly how this is useful besides fixing spam from a rarely useful tool that
Google refuses to fix. Derek tried to ask them to ignore timeouts and they did nothing.