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

[Michael Niedermayer]

On Sun, Nov 08, 2015 at 05:14:10PM +0100, Andreas Cadhalpun wrote:
> If accu overflows, a negative value can be returned.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
>  libavcodec/sbrdsp_fixed.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)

CCing the authors of this, they are probably interrested in
commenting

but the patch does not look like an optimal solution
also does anyone known if values large enough to cause overflows
are alowed in valid AAC ? (didnt investigate yet, just asking as
someone might know ...)

also an additional bit of precission can be use by making the
variable unsigned


> 
> diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c
> index 5b7b7a6..7ab6cc6 100644
> --- a/libavcodec/sbrdsp_fixed.c
> +++ b/libavcodec/sbrdsp_fixed.c
> @@ -35,13 +35,29 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
>  {
>      SoftFloat ret;
>      int64_t accu = 0;
> -    int i, nz, round;
> +    int i, nz, round, exp_offset = 0;
>  
>      for (i = 0; i < n; i += 2) {
> -        accu += (int64_t)x[i + 0][0] * x[i + 0][0];
> -        accu += (int64_t)x[i + 0][1] * x[i + 0][1];
> -        accu += (int64_t)x[i + 1][0] * x[i + 1][0];
> -        accu += (int64_t)x[i + 1][1] * x[i + 1][1];
> +        accu += ((int64_t)x[i + 0][0] * x[i + 0][0]) >> exp_offset;
> +        if (accu > 0x3FFFFFFFFFFFFFFF) {
> +            exp_offset += 1;
> +            accu >>= 1;
> +        }
> +        accu += ((int64_t)x[i + 0][1] * x[i + 0][1]) >> exp_offset;
> +        if (accu > 0x3FFFFFFFFFFFFFFF) {
> +            exp_offset += 1;
> +            accu >>= 1;
> +        }
> +        accu += ((int64_t)x[i + 1][0] * x[i + 1][0]) >> exp_offset;
> +        if (accu > 0x3FFFFFFFFFFFFFFF) {
> +            exp_offset += 1;
> +            accu >>= 1;
> +        }
> +        accu += ((int64_t)x[i + 1][1] * x[i + 1][1]) >> exp_offset;
> +        if (accu > 0x3FFFFFFFFFFFFFFF) {
> +            exp_offset += 1;
> +            accu >>= 1;
> +        }
>      }
>  
>      i = (int)(accu >> 32);
> @@ -59,7 +75,7 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n)
>      round = 1 << (nz-1);
>      i = (int)((accu + round) >> nz);
>      i >>= 1;
> -    ret = av_int2sf(i, 15 - nz);
> +    ret = av_int2sf(i, 15 - nz + exp_offset);
>  
>      return ret;
>  }
> -- 
> 2.6.2
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20151108/f0fad8e1/attachment.sig>