On Wed, Nov 18, 2015 at 7:01 PM, Andreas Cadhalpun <andreas.cadhal...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
