pschatzmann / arduino-liblame

A simple mp3 encoder (not only) for Arduino using LAME
20 stars 3 forks source link

Compiler warnings indicate a possible read from invalid memory #4

Closed zackees closed 1 year ago

zackees commented 1 year ago

From this build log https://github.com/zackees/arduino-liblame/actions/runs/3699790602/jobs/6267520761

We see that functions VBR_new_prepare, VBR_old_iteration_loop and VBR_old_prepare are problematic.

/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c: In function ‘VBR_old_iteration_loop’:
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c:1[54](https://github.com/zackees/arduino-liblame/actions/runs/3699790602/jobs/6267520761#step:5:55)2:22: warning: ‘VBR_old_prepare’ accessing 64 bytes in a region of size [60](https://github.com/zackees/arduino-liblame/actions/runs/3699790602/jobs/6267520761#step:5:61) [-Wstringop-overflow=]
 1542 |     analog_silence = VBR_old_prepare(gfc, pe, ms_ener_ratio, ratio,
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1543 |                                      data->l3_xmin, frameBits, min_bits, max_bits, bands);
      |                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c:1542:22: note: referencing argument 6 of type ‘int *’
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c:1410:1: note: in a call to function ‘VBR_old_prepare’
 1410 | VBR_old_prepare(lame_internal_flags * gfc,
      | ^~~~~~~~~~~~~~~
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c: In function ‘VBR_new_iteration_loop’:
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c:1711:22: warning: ‘VBR_new_prepare’ accessing 64 bytes in a region of size 60 [-Wstringop-overflow=]
 1711 |     analog_silence = VBR_new_prepare(gfc, pe, ratio, data->l3_xmin, frameBits, max_bits, &pad);
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c:1711:22: note: referencing argument 5 of type ‘int *’
/home/runner/work/arduino-liblame/arduino-liblame/src/liblame/quantize.c:1[62](https://github.com/zackees/arduino-liblame/actions/runs/3699790602/jobs/6267520761#step:5:63)1:1: note: in a call to function ‘VBR_new_prepare’
 1621 | VBR_new_prepare(lame_internal_flags * gfc,
      | ^~~~~~~~~~~~~~~

Looking into the source code for VBR_new_prepare we see int framebits[16]:

static int
VBR_new_prepare(lame_internal_flags * gfc,
                const FLOAT pe[2][2], const III_psy_ratio ratio[2][2],
                FLOAT l3_xmin[2][2][SFBMAX], int frameBits[16], int max_bits[2][2],
                int* max_resv)

But when it's called from VBR_new_itereration_loop we see this:

    int     frameBits[15];  // <---- Size 15, but destination function specified int[16]!
    int     used_bits;
    int     max_bits[2][2];
    int     ch, gr, analog_silence, pad;
    III_side_info_t *const l3_side = &gfc->l3_side;

    const FLOAT (*const_l3_xmin)[2][SFBMAX] = (const FLOAT (*)[2][SFBMAX])data->l3_xmin;
    const FLOAT (*const_xrpow)[2][576] = (const FLOAT (*)[2][576])data->xrpow;
    const int (*const_max_bits)[2] = (const int (*)[2])max_bits;

    (void) ms_ener_ratio; /* not used */

    memset(data->xrpow, 0, sizeof(data->xrpow));

    analog_silence = VBR_new_prepare(gfc, pe, ratio, data->l3_xmin, frameBits, max_bits, &pad);
zackees commented 1 year ago

Looking at the code further it looks like there maximum index is 14 so this is just a compiler warning that can be ignored.

pschatzmann commented 1 year ago

This is part of the original code and has not been changed by me

zackees commented 1 year ago

I've verified that this the memory in question cannot be accessed. The parameter in question specifies a float[16] but a float[15] is passed in, triggering the warning. However, it appears from examining the code that the index only goes as high as 14, so float[15] should be fine here.

Closing.