On 14.12.2015 02:38, Michael Niedermayer wrote:
> On Sun, Dec 13, 2015 at 10:51:31PM +0100, Andreas Cadhalpun wrote:
>> nlsf can be negative, but a negative index for silk_cosine doesn't work.
>> ---
>> libavcodec/opus_silk.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/opus_silk.c b/libavcodec/opus_silk.c
>> index 841d1ed..3ac83b8 100644
>> --- a/libavcodec/opus_silk.c
>> +++ b/libavcodec/opus_silk.c
>> @@ -941,7 +941,7 @@ static void silk_lsf2lpc(const int16_t nlsf[16], float
>> lpcf[16], int order)
>>
>> /* convert the LSFs to LSPs, i.e. 2*cos(LSF) */
>> for (k = 0; k < order; k++) {
>> - int index = nlsf[k] >> 8;
>> + int index = FFABS(nlsf[k]) >> 8;
>> int offset = nlsf[k] & 255;
>
> this looks a bit strange
>
> if nlsf[] is allowed to be negative then i would have expected that
> both nlsf[] would use the absolute value or if its nt allowed to be
> negative then why is it ?
Actually the specification says it shouldn't be negative.
It's negative due to an overflow, when an intermediary uint16_t value
larger than INT16_MAX is assigned to nlsf.
> does the spec explain what should be done in this case ?
Not directly, but it implies that the overflow shouldn't happen [1]:
Then, for each
value of k from 0 to d_LPC-1, NLSF_Q15[k] is set to
max(NLSF_Q15[k], NLSF_Q15[k-1] + NDeltaMin_Q15[k])
Next, for each value of k from d_LPC-1 down to 0, NLSF_Q15[k] is set
to
min(NLSF_Q15[k], NLSF_Q15[k+1] - NDeltaMin_Q15[k+1])
> if not, its authors may want to amend it to clarify if this is
> invalid or undefined or something specific should be done
I think the specification is fine, just not our code.
Attached is a patch fixing this.
Best regards,
Andreas
1: https://tools.ietf.org/html/rfc6716
>From 958789a66e6f55e05ab3d8e945b8ff899680c073 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Mon, 14 Dec 2015 20:31:41 +0100
Subject: [PATCH] opus_silk: fix int16_t overflow in silk_stabilize_lsf
nlsf[i - 1] + min_delta[i] can be larger than INT16_MAX, causing nlsf to
be set to a negative value. However, it is not supposed to be negative
and if it is, it causes an out of bounds read in silk_lsf2lpc.
Since min_delta is unsigned, the overflow only happens when the result
of the addition is assigned to nlsf, so that the FFMIN solves the
problem.
Even though the specification implies that the value of nlfs can be
larger than INT16_MAX at this intermediary point, it is reduced to the
int16_t range in the next loop, the result of which doesn't change if
the too large intermediary values are replaced by INT16_MAX.
Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
libavcodec/opus_silk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/opus_silk.c b/libavcodec/opus_silk.c
index 841d1ed..491832f 100644
--- a/libavcodec/opus_silk.c
+++ b/libavcodec/opus_silk.c
@@ -852,7 +852,7 @@ static inline void silk_stabilize_lsf(int16_t nlsf[16], int order, const uint16_
nlsf[0] = min_delta[0];
for (i = 1; i < order; i++)
if (nlsf[i] < nlsf[i - 1] + min_delta[i])
- nlsf[i] = nlsf[i - 1] + min_delta[i];
+ nlsf[i] = FFMIN(nlsf[i - 1] + min_delta[i], INT16_MAX);
/* push backwards to increase distance */
if (nlsf[order-1] > 32768 - min_delta[order])
--
2.6.2
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
