Speex dev - Sep 2007 - Possible fixed point overflow/div 0 preprocess.c

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

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@xiph.org
> http://lists.xiph.org/mailman/listinfo/speex-dev
>