[FFmpeg-devel] [PATCH] sbr_qmf_analysis: sanitize input for 32-bit imdct

[Andreas Cadhalpun]

On 19.11.2015 01:02, Andreas Cadhalpun wrote:
> If the input contains too many too large values, the imdct can overflow.
> Even if it didn't, the output would be larger than the valid range of 29
> bits.
> 
> Note that this is a very delicate limit: Allowing values up to 1<<25
> does not prevent input larger than 1<<29 from arriving at
> sbr_sum_square, while limiting values to 1<<23 breaks the
> fate-aac-fixed-al_sbr_hq_cm_48_5.1 test.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
> 
> Nedeljko, do you have an explanation why larger input values here
> are invalid?
> 
> By the way, the imdct calculations in imdct_and_windowing and
> sbr_qmf_synthesis can also overflow, so maybe need a similar check.
> 
> ---
>  libavcodec/aacsbr_template.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/libavcodec/aacsbr_template.c b/libavcodec/aacsbr_template.c
> index 66f4159..f48ddd0 100644
> --- a/libavcodec/aacsbr_template.c
> +++ b/libavcodec/aacsbr_template.c
> @@ -1153,6 +1153,9 @@ static void sbr_qmf_analysis(AVFloatDSPContext *dsp, FFTContext *mdct,
>                               INTFLOAT z[320], INTFLOAT W[2][32][32][2], int buf_idx)
>  {
>      int i;
> +#if USE_FIXED
> +    int j;
> +#endif
>      memcpy(x    , x+1024, (320-32)*sizeof(x[0]));
>      memcpy(x+288, in,         1024*sizeof(x[0]));
>      for (i = 0; i < 32; i++) { // numTimeSlots*RATE = 16*2 as 960 sample frames
> @@ -1160,6 +1163,21 @@ static void sbr_qmf_analysis(AVFloatDSPContext *dsp, FFTContext *mdct,
>          dsp->vector_fmul_reverse(z, sbr_qmf_window_ds, x, 320);
>          sbrdsp->sum64x5(z);
>          sbrdsp->qmf_pre_shuffle(z);
> +#if USE_FIXED
> +        for (j = 64; j < 128; j++) {
> +            if (z[j] > 1<<24) {
> +                av_log(NULL, AV_LOG_WARNING,
> +                       "sbr_qmf_analysis: value %09d too large, setting to %09d\n",
> +                       z[j], 1<<24);
> +                z[j] = 1<<24;
> +            } else if (z[j] < -(1<<24)) {
> +                av_log(NULL, AV_LOG_WARNING,
> +                       "sbr_qmf_analysis: value %09d too small, setting to %09d\n",
> +                       z[j], -(1<<24));
> +                z[j] = -(1<<24);
> +            }
> +        }
> +#endif
>          mdct->imdct_half(mdct, z, z+64);
>          sbrdsp->qmf_post_shuffle(W[buf_idx][i], z);
>          x += 32;
> 

Ping.

If nobody objects, I'll push this soon, as it fixes crashes.

Best regards,
Andreas