[FFmpeg-devel] [PATCH] aac_fixed: fix overflow in sbr_sum_square_c

[Andreas Cadhalpun]

On 13.11.2015 04:15, Michael Niedermayer wrote:
> On Thu, Nov 12, 2015 at 08:43:42PM +0100, Andreas Cadhalpun wrote:
>> Considering that the aac float decoder can decode such samples, I tend
>> to think that the aac fixed decoder should be able to do that, too.
> 
> IMO this reasoning is flawed
> 
> if you write a float mpeg2 decoder and feed it a fuzzed file and
> that decoder skips all checks then you can get  >16bit coefficients
> as input to the IDCT.
> Changing the fixed point IDCTs to work with 32x32 bit mutiplies
> would not fix anything it would just make everything much slower
> 
> either way the authors of the fixed point aac decoder should
> decide on what needs to be done, they implemented it and know this
> code much better and why it scales the values as they are scaled ...

Yes, it would be great if they could comment on this.

>> If the FFmpeg aac encoder produces valid aac files then, yes, lots of
>> overflows can happen with valid aac files.
>> The decoder overflows even with a FATE sample.
> 
> i think this is something the authors of the decoder should look
> into 

OK.

> also if there are overflows with fate samples why does this not
> show up on any test on fate.ffmpeg.org ?

Because these samples aren't tested with the aac_fixed decoder:
 * aac/ct_faac-adts.aac is only used to test the aac demuxer.
 * aac/al07_96.mp4 is for some reason only tested with the aac decoder.

There the overflow happens on lines like:
che->ch[0].ret[j] = (int32_t)av_clipl_int32((int64_t)che->ch[0].ret[j]<<7)+0x8000;

I guess the +0x8000 was meant to be inside av_clipl_int32.

Best regards,
Andreas