On Mon, Dec 14, 2015 at 08:43:38PM +0100, Andreas Cadhalpun wrote:
> 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> opus_silk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > cc0c02e14c1bda0ab35813c8d4629e742af7d23f > 0001-opus_silk-fix-int16_t-overflow-in-silk_stabilize_lsf.patch > 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(-) should be ok thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
