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

[Ganesh Ajjanagadde]

On Wed, Nov 18, 2015 at 7:01 PM, Andreas Cadhalpun
<andreas.cadhalpun at gmail.com> wrote:
> On 16.11.2015 15:39, Nedeljko Babic wrote:
>>>> On 11.11.2015 13:46, Michael Niedermayer wrote:
>>> Comments fro AAC and SBR experts very welcome!
>>
>> This code was developed a while ago, but based on informations that I have
>> this part of code was analysed regarding possibility of overflow and conclusion
>> was that there is no valid way for causing overflow here.
>
> I would be very interested in details about this analysis of yours.
> My investigation of this code leads me to believe that actually the potential
> input range for sbr_sum_square is several orders of magnitude larger than what
> fits into an int32_t, but since that type is used, lots of overflows are happening,
> most during the imdct calculation.
>
>> And regarding valid range, if I remember correctly it should be 29, not 32.
>
> Well, the input range should be 29-bits, because otherwise this function can
> overflow.
>
> So if you say that larger values are invalid, I suggest to assert that they
> don't happen. See attached patch.
>
> To prevent triggering these asserts, one can force the input to be small enough.
> Doing that early enough also avoids overflows along the way.
> I'll send a separate patch for that.

I have not analyzed any of this stuff, but just a general suggestion
to all here based on these aac related threads:
Please document any weird ranges like the 29 bits above in the code
either as asserts or as comments. It helps a lot in future analysis
for an unfamiliar reader in verifying lack of overflow, etc.
I mention this due to some back and forth I had regarding apedec:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/180990.html.
There it is entirely possible that I was just dumb and could not see
the 24 bit thing easily, but here clearly in spite of a lot of thought
the analysis is hard.

>
> Best regards,
> Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>