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

[Andreas Cadhalpun]

On 02.12.2015 20:58, Andreas Cadhalpun wrote:
> 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.

I pushed this now.

Best regards,
Andreas