[Speex-dev] Possible fixed point overflow/div 0 preprocess.c

[Jean-Marc Valin]

Hi Peter,

Thanks a lot for posting this bug report. There's indeed a problem with 
the fixed-point code, though not in the floating point (it turns out 
that "floating-point exception" is very misleading as it only occurs on 
*integer* divisions, not float). Anyway, so the fix should be fairly 
simple. I'll fix that in the next few days -- flame me if I forget.

Cheers,

	Jean-Marc

Peter Rowling wrote:
> Hi,
> 
> I'll try to keep this as brief, yet descriptive enough to save everyone
> some time. 
> If I'm off with this one please forgive me, but it has fixed my issues.
> I am not
> sure whether it is just my compiler (gcc 3.3.5) doing the wrong thing
> with the cast.
> 
> File: preprocess.c
> Arch affected: x86, (others?)
> svn revision: 12778
> 
> Description:
> 
> The SpeexPreprocessState_ member 'nb_adapt' is a signed integer.
> nb_adapt is only
> modified during 'speex_preprocess_run' when an unbounded increment is
> performed. When
> calculating 'beta' within 'speex_preprocess_run', I received a floating
> point exception
> even when compiled with -DFIXED_POINT.  The reason is as follows:
> 
> <original beta calculation within speex_preprocess_run>
> ...
>    st->nb_adapt++;
>    st->min_count++;
>    
>    beta = MAX16(QCONST16(.03,15),DIV32_16(Q15_ONE,st->nb_adapt));
>    beta_1 = Q15_ONE-beta;
> ...
> </>
> 
> On my architecture(x86) at least the DIV32_16 is defined as follows.
> 
> typedef short spx_int16_t;
> typedef spx_int16_t spx_word16_t;
> #define DIV32_16(a,b)
> ((spx_word16_t)(((spx_word32_t)(a))/((spx_word16_t)(b))))
> 
> The nb_adapt overflow occurred when its value reached 0x00010000 when
> the cast is
> performed to a short the value 0x0000 is given and hence the divide by
> zero exception.
> 
> Proposed resolution:
> 
> The SpeexPreprocessState_ member 'nb_adapt' is only checked in a couple
> of places and
> mostly for initial conditions such as within update_noise_prob below:
> 
> <preprocess.c update_noise_prob>
> ...
>    if (st->nb_adapt==1)
>    {
>       for (i=0;i<N;i++)
>          st->Smin[i] = st->Stmp[i] = 0;
>    }
> 
>    if (st->nb_adapt < 100)
>       min_range = 15;
>    else if (st->nb_adapt < 1000)
>       min_range = 50;
>    else if (st->nb_adapt < 10000)
>       min_range = 150;
>    else
>       min_range = 300;
> ...
> </>
> 
> Maybe something like this is required within speex_preprocess_run to
> prevent the observed overflow.
> 
> <preprocess.c speex_preprocess_run>
> ...
>    if ( st->nb_adapt < 0x7FFF /* or 32767 as elsewhere in the code */ )
>        st->nb_adapt++;
> ...
> </>
> 
> Thanks.
> 
> Peter Rowling
> 
> _______________________________________________
> Speex-dev mailing list
> Speex-dev at xiph.org
> http://lists.xiph.org/mailman/listinfo/speex-dev
>