Menu

#279 tan(pi/2)=inf and 1/0 in layer3.c, function init_layer3_gainpow2

0.68
closed-fixed
nobody
None
5
2021-03-14
2019-09-18
iakai
No

Hi,
in layer3.c the code for the function init_layer3_gainpow2 contains the following lines:
for(i=0;i<16;i++)
{
double t = tan( (double) i * M_PI / 12.0 );
tan1_1[i] = DOUBLE_TO_REAL_15(t / (1.0+t));
tan2_1[i] = DOUBLE_TO_REAL_15(1.0 / (1.0 + t));
tan1_2[i] = DOUBLE_TO_REAL_15(M_SQRT2 * t / (1.0+t));
tan2_2[i] = DOUBLE_TO_REAL_15(M_SQRT2 / (1.0 + t));
}
For i=6 this results in t=tan(pi/2)= inf.
For i=9 t evaluates to t=-1,
The following 4 lines of code contain 1/(1+t) resulting in 1/0.
Cases i=12 to 15 repeat the first 4 values.
Are the cases t=inf and 1/(1+t)=1/0 handled correctly in this program ?

Regards iakai

Discussion

  • Thomas Orgis

    Thomas Orgis - 2019-09-19

    Good find. From a glance at my precedessor's code, I don't see that the cases are excluded in use. The indices to use are determined in III_get_scale_factors_1() and III_get_scale_factors_2(), where tables are constructed in which lookup for the lookup indices is done later (yes, some bits of indirection here).

    I have the suspicion that you could craft a stream that then produces NaNs as output. But this did not occur yet during years of testing (also with fuzzers). So I am willing to consider that I overlooked something. It could be more relevant for float decoder output.

    This needs some calm investigation …

     
  • iakai

    iakai - 2019-09-19

    One way to handle these cases more safely would be seperate handling of i=6 and i=9. In the following lines the tan(pi/2)=inf problem collapses to 1/inf=0 and inf/inf=1 and -inf/inf=-1. So it is better to set tan1_1, tan1_2, tan2_1 & tan2_2 directly without using tan(pi/2). Some compilers might generate an exception, others might just set tan(pi/2)=maxfloat. Case i=9 can be handled in a similar fashion: The four tan_ variables evaluate to +- inf, which might be replaced by +-maxfloat if subsequent code can do calculations with these values without generating exceptions (overflow, underflow ...):

     

    Last edit: iakai 2019-09-19
  • Thomas Orgis

    Thomas Orgis - 2019-10-16

    Well, this is what happens:

    0 t=0, 0, 1, 0, 1.41421
    1 t=0.267949, 0.211325, 0.788675, 0.298858, 1.11536
    2 t=0.57735, 0.366025, 0.633975, 0.517638, 0.896575
    3 t=1, 0.5, 0.5, 0.707107, 0.707107
    4 t=1.73205, 0.633975, 0.366025, 0.896575, 0.517638
    5 t=3.73205, 0.788675, 0.211325, 1.11536, 0.298858
    6 t=1.63312e+16, 1, 6.12323e-17, 1.41421, 8.65956e-17
    7 t=-3.73205, 1.36603, -0.366025, 1.93185, -0.517638
    8 t=-1.73205, 2.36603, -1.36603, 3.34607, -1.93185
    9 t=-1, 4.5036e+15, -4.5036e+15, 6.36905e+15, -6.36905e+15
    10 t=-0.57735, -1.36603, 2.36603, -1.93185, 3.34607
    11 t=-0.267949, -0.366025, 1.36603, -0.517638, 1.93185
    12 t=-1.22465e-16, -1.22465e-16, 1, -1.73191e-16, 1.41421
    13 t=0.267949, 0.211325, 0.788675, 0.298858, 1.11536
    14 t=0.57735, 0.366025, 0.633975, 0.517638, 0.896575
    15 t=1, 0.5, 0.5, 0.707107, 0.707107
    

    We just get big values, and small values. I did not manage to get the compiler to complain. Let's quote the man page:

           If the correct result would overflow, a range error occurs, and  the  functions  return
           HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively, with the mathematically correct sign.
    

    I still need to see an example that triggers usage of these values in a way to produce distorted output. I have the suspicion that 'very big' and 'very small' just fit into the actual computation and result in normal sample values.

    If I stress the point and enable floating point exceptions using feenableexcept(), I do get a core dump. So ... if a client program that uses libmpg123 explicitly asks for the crash, it will happen. Given that you have to be bring your own prototype to be able to call feenableexcept(), this seems to be rare.

    I guess that is a reason to play safe and just special-case those values. But I am not sure if we got other places in libmpg123 where there is reliance on a HUGE_VAL result, for example.

     
  • Thomas Orgis

    Thomas Orgis - 2020-03-15

    So far I did nothing about this. I am contemplating getting rid of the run-time computation of these tables. Need to be careful to provide float and fixed variants, but there isn't strong reason to not hardcode the numbers. Need to check if setting theoretical infinity to HUGE_VAL or any other value does actually influence things.

    I'd like to settle this together with getting rid of mpg123_init(), but after releasing 1.26.0. It's too long overdue.

     
  • Thomas Orgis

    Thomas Orgis - 2020-12-13

    I finally committed this in rev 4770. There's a hollow feeling because the concerned values seem to truly be irrelevant, but I don't easily can tell that from the code. Some proper analysis probably should show that the values indeed are never touched by valid streams. But maybe by corrupted ones …

     
  • Thomas Orgis

    Thomas Orgis - 2021-03-14
    • status: open --> closed-fixed
     

Log in to post a comment.