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

[Michael Niedermayer]

On Fri, Nov 13, 2015 at 10:32:47PM +0100, Andreas Cadhalpun wrote:
> 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.

could be
also there was a different patch about a overflow in that
0729 11:58 Nedeljko Babic  (1.8K) [FFmpeg-devel] [PATCH] avcodec/aacdec_fixed: Fix integer overflow
that case was a bug elsewhere though

in that light, my first question, is, is the overflowing value used
(aka affects the decoder output) ?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151116/bfa71a65/attachment.sig>