#202 potentially uninitialized local variable in FilterBank

[Alexander Riccio]

I think this is a false positive, but the code could certainly be improved.

Building in Visual Studio 2013 with SDL checks produces:

libfaac\filtbank.c(161): warning C4701: potentially uninitialized local variable 'first_window' used
libfaac\filtbank.c(161): error C4703: potentially uninitialized local pointer variable 'first_window' used
\libfaac\filtbank.c(162): warning C4701: potentially uninitialized local variable 'second_window' used
\libfaac\filtbank.c(162): error C4703: potentially uninitialized local pointer variable 'second_window' used

Both variables are declared, C89 style, at the top of the function:

void FilterBank(faacEncHandle hEncoder,
                CoderInfo *coderInfo,
                double *p_in_data,
                double *p_out_mdct,
                double *p_overlap,
                int overlap_select)
{
    double *p_o_buf, *first_window, *second_window;

...and conditionally assigned in a few large blocks:

    /*  Window shape processing */
    if(overlap_select != MNON_OVERLAPPED) {
        switch (coderInfo->prev_window_shape) {
        case SINE_WINDOW:
            if ( (block_type == ONLY_LONG_WINDOW) || (block_type == LONG_SHORT_WINDOW))
                first_window = hEncoder->sin_window_long;
            else
                first_window = hEncoder->sin_window_short;
            break;
        case KBD_WINDOW:
            if ( (block_type == ONLY_LONG_WINDOW) || (block_type == LONG_SHORT_WINDOW))
                first_window = hEncoder->kbd_window_long;
            else
                first_window = hEncoder->kbd_window_short;
            break;
        }

        switch (coderInfo->window_shape){
        case SINE_WINDOW:
            if ( (block_type == ONLY_LONG_WINDOW) || (block_type == SHORT_LONG_WINDOW))
                second_window = hEncoder->sin_window_long;
            else
                second_window = hEncoder->sin_window_short;
            break;
        case KBD_WINDOW:
            if ( (block_type == ONLY_LONG_WINDOW) || (block_type == SHORT_LONG_WINDOW))
                second_window = hEncoder->kbd_window_long;
            else
                second_window = hEncoder->kbd_window_short;
            break;
        }
    } else {
        /* Always long block and sine window for LTP */
        first_window = hEncoder->sin_window_long;
        second_window = hEncoder->sin_window_long;
    }

...before being read:

    /* Separate action for each Block Type */
    switch (block_type) {
    case ONLY_LONG_WINDOW :
        for ( i = 0 ; i < BLOCK_LEN_LONG ; i++){
            p_out_mdct[i] = p_o_buf[i] * first_window[i];
            p_out_mdct[i+BLOCK_LEN_LONG] = p_o_buf[i+BLOCK_LEN_LONG] * second_window[BLOCK_LEN_LONG-i-1];
        }
        MDCT( &hEncoder->fft_tables, p_out_mdct, 2*BLOCK_LEN_LONG );
        break;

...for which two lines generate the error:

            p_out_mdct[i] = p_o_buf[i] * first_window[i];
            p_out_mdct[i+BLOCK_LEN_LONG] = p_o_buf[i+BLOCK_LEN_LONG] * second_window[BLOCK_LEN_LONG-i-1];

...in order to actually assign from uninit data, any of:
1. coderInfo->prev_window_shape is neither SINE_WINDOW nor KBD_WINDOW. (I don't think this happens anywhere, BUT this might be throwing MSVC off)
2. Allocation of either hEncoder->kbd_window_long or hEncoder->kbd_window_shortfails, thus they're NULL (there's NO checking!)
3. hEncoder->sin_window_short or hEncoder->kbd_window_longis uninitialized. (more on this in a second)

There's something wacky going on with hEncoder->kbd_window_long and hEncoder->kbd_window_short: They're both initialized in CalculateKBDWindow, in two loops:

static void CalculateKBDWindow(double* win, double alpha, int length)
{
    int i;
    double IBeta;
    double tmp;
    double sum = 0.0;

    alpha *= M_PI;
    IBeta = 1.0/Izero(alpha);

    /* calculate lower half of Kaiser Bessel window */
    for(i=0; i<(length>>1); i++) {
        tmp = 4.0*(double)i/(double)length - 1.0;
        win[i] = Izero(alpha*sqrt(1.0-tmp*tmp))*IBeta;
        sum += win[i];
    }

    sum = 1.0/sum;
    tmp = 0.0;

    /* calculate lower half of window */
    for(i=0; i<(length>>1); i++) {
        tmp += win[i];
        win[i] = sqrt(tmp*sum);
    }
}

...the length>>1 means that win will only be half initialized!

Thus, the two erroneous lines that caused the error:

            p_out_mdct[i] = p_o_buf[i] * first_window[i];
            p_out_mdct[i+BLOCK_LEN_LONG] = p_o_buf[i+BLOCK_LEN_LONG] * second_window[BLOCK_LEN_LONG-i-1];

...can read from half of an uninitialized buffer!

Sidenote: the proper SAL, if anybody's interested (probably not), for CalculateKBDWindow would be:

static void CalculateKBDWindow(_Out_writes_(length/2) double* win, double alpha, int length)

...but since CalculateKBDWindow is called as:

    CalculateKBDWindow(hEncoder->kbd_window_long, 4, BLOCK_LEN_LONG*2);
    CalculateKBDWindow(hEncoder->kbd_window_short, 6, BLOCK_LEN_SHORT*2);

...this doesn't matter, but is probably unnecessarily confusing, since those are the only two calls to CalculateKBDWindow.

---

Suggested fix:

Error out/abort() in a default case of switch (coderInfo->prev_window_shape) and switch (coderInfo->window_shape) . This gets rid of the warning, and I imagine that if you're compiling with an optimizer that's worth it's weight in Software Engineers, this'll get eliminated in the data flow analysis pass.

---

Side note: have you build libfaac with any kind of static analysis tool? There're TONS of warnings generated by MSVC's /analyze.