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

[Michael Niedermayer]

On Thu, Nov 12, 2015 at 08:43:42PM +0100, Andreas Cadhalpun wrote:
> On 11.11.2015 22:52, Michael Niedermayer wrote:
> > On Wed, Nov 11, 2015 at 09:09:51PM +0100, Andreas Cadhalpun wrote:
> >> On 11.11.2015 13:46, Michael Niedermayer wrote:
> >>> On Sun, Nov 08, 2015 at 09:26:21PM +0100, Andreas Cadhalpun wrote:
> >>>> On 08.11.2015 20:17, Michael Niedermayer wrote:
> >>>>> but the patch does not look like an optimal solution
> >>>>
> >>>> It's certainly not pretty, but it fixes the crashes/assertion failures.
> >>>
> >>> at the expense of making the code rather slow and hard to implement in
> >>> SIMD.
> >>> This would be perfectly ok if that is neccessary but is it ?
> >>> (i dont know)
> >>
> >> That depends how you define necessary. It's possible to avoid the problem
> >> with simpler, faster means, but those are less correct mathematically.
> > 
> > I define neccessary as what is neccessary for a well working fixed
> > point aac decoder.
> > 
> > There are 2 possible cases i think
> > A. The large input values are invalid then the correct solution is
> >    to detect that and error out / do error concealment
> > B. The large values are valid, in this case the implementation is
> >    broken. And the question would arrise how to best support the valid
> >    range, can the values or their products be scaled down by a fixed
> >    shift? or does this affect quality, do we need a variable shift
> >    if so how to implement this best (like 2pass, find max, choose
> >    shift at a earlier stage possibly, some kind of floats, ...)
> 
> 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 ...


> 
> >>> do we know the valid input range?
> >>
> >> I don't know about valid, but the possible input range currently seems
> >> to be any 32-bit integer.
> > 
> > yes, and neither do i know the valid range but
> > SBR builds the high frequency signal and adds that to the base AAC
> > low frequency signal. These cannot be arbitrary large as the output
> > cannot be arbitrary large so there is a practical limit on how large
> > these values can be unless its possible and allowed to have much
> > larger intermediates at some point.
> 
> Well, in this case the intermediates can be much larger, because the
> further calculation involves something like the square root of the inverse
> of the value calculated by the function in question.
> (And now we're back to the cause of this investigation: calculating
> the square root of a negative value.)
> 
> > [...]
> >>> or maybe "valid" is not the correct word (in case the spec does not
> >>> specify that directly or indirectly ...)
> >>> if thats the case then it could be a question if any encoder could
> >>> ever have a reason to use values in a specific range
> >>
> >> Anyway, the decoder has to deal with such values somehow.
> >> It could also error out, but I don't know which error check to use.
> >> And it would be a bit strange if the aac_fixed decoder errors out,
> >> while the float aac decoder can handle such samples.
> > 
> > can you check if there are any overflows happening in valid aac files
> > ?
> 

> 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 
also if there are overflows with fate samples why does this not
show up on any test on fate.ffmpeg.org ?


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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- 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/20151113/0fa84fb4/attachment.sig>