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.
Best regards,
Andreas
>From 0555a4aa4a9398e7abc787760b67032fbb00e7e6 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Thu, 19 Nov 2015 00:37:52 +0100
Subject: [PATCH] sbrdsp_fixed: assert that input values for sbr_sum_square_c
are valid
Larger values can cause overflows, leading to this function returning a
negative value.
Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
libavcodec/sbrdsp_fixed.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
index 5b7b7a6..f4e3de0 100644
--- a/libavcodec/sbrdsp_fixed.c
+++ b/libavcodec/sbrdsp_fixed.c
@@ -38,9 +38,14 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
int i, nz, round;
for (i = 0; i < n; i += 2) {
+ // Larger values are inavlid and could cause overflows of accu.
+ av_assert2(FFABS(x[i + 0][0]) >> 29 == 0);
accu += (int64_t)x[i + 0][0] * x[i + 0][0];
+ av_assert2(FFABS(x[i + 0][1]) >> 29 == 0);
accu += (int64_t)x[i + 0][1] * x[i + 0][1];
+ av_assert2(FFABS(x[i + 1][0]) >> 29 == 0);
accu += (int64_t)x[i + 1][0] * x[i + 1][0];
+ av_assert2(FFABS(x[i + 1][1]) >> 29 == 0);
accu += (int64_t)x[i + 1][1] * x[i + 1][1];
}
--
2.6.2
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
