Re: [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 
>> ---
>>
>> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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 
> ---
> 
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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 
---

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;
-- 
2.6.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel