#233 fix cppcheck array index out of bounds warnings

[Andre Maute]

Using a Fedora 33 with a not so recent cppcheck, and based on commit hash 37fac7

$ cppcheck --version
Cppcheck 2.3

$ cppcheck --force --xml-version=2 --enable=warning -I/usr/include ./backend 2>cppcheck.xml
(needs approximately 3 hours)

$ cat cppcheck.xml | grep -A3 -ie "arrayindexoutofbounds" > todo-out-of-bounds.txt

The todo file contains 7 issues.
The todo file doesn't contain the recent reported issue in code1.c.
So would have been supposedly not be found with cppcheck 2.3

[Andre Maute]

I missed the following one due to a different warning type

        <error id="negativeIndex" severity="warning" msg="Either the condition &apos;CompactAztecMap[r+x]&gt;=2&apos; is redundant or the array &apos;binary_string[28]&apos; is accessed at index -1998, which is out of bounds." verbose="Either the condition &apos;CompactAztecMap[r+x]&gt;=2&apos; is redundant or the array &apos;binary_string[28]&apos; is accessed at index -1998, which is out of bounds." cwe="786" hash="17691147984548352444">
            <location file="backend/aztec.c" line="1488" column="68" info="Negative array index"/>
            <location file="backend/aztec.c" line="1488" column="47" info="Assuming that condition &apos;CompactAztecMap[r+x]&gt;=2&apos; is not redundant"/>
        </error>

[Git Lost]

Hi, I think commit [4d3aae] probably addresses most of these.

There was one definite bug found, in "testcommon.c", where it was using sizeof() instead of ARRAY_SIZE().

The AZTEC warning was a nicety (only need to check not zero instead of >= 2) but good to fix.

The QR warnings I'd consider dubious - was using non-contiguous version indexes for RMQR and MICROQR (which doesn't matter) - but changed them anyway to be contiguous. Likewise the "test_gb18030" comparison was redundant (but sometimes redundancy is ok) - but commented it out anyway.

Unfortunately I found running "cppcheck" - tried various versions, 2.5, 2.4, 2.3 - so slow as to be close to unusable. Only ran it on the individual files in your report, and gave up trying to run it on "testcommon.c", it was taking so long (and crucifying my machine).

Thanks, Martin

[Andre Maute]

I have checked again (without the --force parameter)
now the following two warnings don't look that nice in the cppcheck output

    <error id="uninitvar" severity="error" msg="Uninitialized variable: fillings" verbose="Uninitialized variable: fillings" cwe="457" hash="12820924118537979900">
        <location file="zint/backend/codablock.c" line="428" column="16"/>
        <symbol>fillings</symbol>
    </error>
   <error id="negativeIndex" severity="warning" msg="Either the condition &apos;source&gt;127&apos; is redundant or the array &apos;C128Table[107]&apos; is accessed at index -32, which is out of bounds." verbose="Either the condition &apos;source&gt;127&apos; is redundant or the array &apos;C128Table[107]&apos; is accessed at index -32, which is out of bounds." cwe="786" hash="9389163874767712819">
        <location file="zint/backend/code128.c" line="241" column="31" info="Negative array index"/>
        <location file="zint/backend/code128.c" line="240" column="16" info="Assuming that condition &apos;source&gt;127&apos; is not redundant"/>
    </error>

[Git Lost]

I suppressed the codablock with commit [7cd0d9].

Tried a few things to suppress the code128 one but it looks like a false positive to me, e.g. changing the test to source > 127 + 32 gives the warning that C128Table[107] accessed at index 127, which is out of bounds, which given that source is < 256 isn't possible.

Edit: oh I missed that changes the line to 244, so it is possible. Sticking a source <= 127 around that fixes that but don't think this is adding anything to the source...

[Git Lost]

Hi, after sleeping on it commit [f03da2] suppresses the code128 warning, plus some others.

Running cppcheck without --force makes a big difference time-wise, though still very CPU intensive.

[Andre Maute]

So what happens if we have 0 <= source <= 31, this would give source-32 <= 31 - 32 = -1
and the array index again is negative. There might be something missing here.

static int c128_set_b(const unsigned char source, char dest[], int values[], int *bar_chars) {
    if (source > 127 + 32) { /* Suppress cppcheck out-of-bounds warning with + 32 check */
        strcat(dest, C128Table[source - 32 - 128]);
        values[(*bar_chars)] = source - 32 - 128;
    } else if (source <= 127) { /* Suppress cppcheck out-of-bounds warning with <= 127 check */
        strcat(dest, C128Table[source - 32]);
        values[(*bar_chars)] = source - 32;
    } else { /* Should never happen */
        return 0; /* Not reached */
    }
    (*bar_chars)++;
    return 1;
}

[Git Lost]

source is never < 32 and never > 127 and < 127 + 32 for Set B, which is why the warning is somewhat dubious and the suppression a bit OTT.

[Andre Maute]

This might be true, but a static analysis tool might not get that.
So why not introduce for each interval a separate if-else-branch,
and mark the unreachable ones as already done?
Perhaps one could use some early returns here in order to remove some if-else clutter.

[Git Lost]

Well the unreachable ones aren't already done, if I get your meaning, they would be an internal programming error, and the callers of c128_set_b() should really check the return value and crap out if false, which I didn't bother to do here (but may do in the future), but MRs for code style improvements are always welcome!

[Andre Maute]

Your're welcome :-)

Regarding the suggestion for a MR.
I still don't get the middle branch?
So let's try it here. Would that work?
I hope I understood your condition correctly.
This would now be a no-brainer for me
because I can easily see now the nonnegativity of the array indices used.

static int c128_set_b(const unsigned char source, char dest[], int values[], int *bar_chars) {
    if (source >= 128 + 32) {
        strcat(dest, C128Table[source - 32 - 128]);
        values[(*bar_chars)] = source - 32 - 128;
    } else if (source >= 128) { /* Should never happen */
        return 0; /* Not reached */
    } else if (source >= 32) {
        strcat(dest, C128Table[source - 32]);
        values[(*bar_chars)] = source - 32;
    } else { /* Should never happen */
        return 0; /* Not reached */
    }
    (*bar_chars)++;
    return 1;
}

Regards Andre

[Git Lost]

Patch applied [e1f22e], much nicer!

[Andre Maute]

Thank you Martin for the fixes.
This Ticket can be closed now.