cppcheck 2.4.1: false positive: arrayIndexOutOfBoundsCond (introduced by generic valueflowBeforeCondition)

[Michał Kępień]

Hi there,

Thank you again for your work on cppcheck!

For the following C program:

struct s {
    int array[1];
    int index;
};

int main(void) {
    struct s foo = { 0 };
    foo.array[foo.index] = 1;
    if (foo.index == 1) {
        return foo.index;
    }
    return 0;
}

cppcheck 2.4.1 (with --enable=warning) reports:

Checking test.c ...
test.c:8:11: warning: Either the condition 'foo.index==1' is redundant or the array 'foo.array[1]' is accessed at index 1, which is out of bounds. [arrayIndexOutOfBoundsCond]
 foo.array[foo.index] = 1;
          ^
test.c:9:16: note: Assuming that condition 'foo.index==1' is not redundant
 if (foo.index == 1) {
               ^
test.c:8:11: note: Array index out of bounds
 foo.array[foo.index] = 1;
          ^

Of course the sample program itself is silly and does not serve any real purpose, it is just the simplest piece of code that I was able to trigger this false positive with.

Applying any of the following changes prevents the false positive from being triggered:

• replacing foo.index == 1 with foo.index == 0
• removing the if (foo.index == ...) condition altogether
• using local variables for index and value instead of structure fields

git bisect claims that commit c267d85640523c045c7d43ba7ce9c0f305423c5d, i.e. pull request #3001, is the culprit. I also carried out an additional bisect on that pull request's branch (pfultz2/generic-before-condition) and it pointed at the first commit on that branch, 811e3d858f854ca574500c907832a4aca2fbb60d ("Add generic before condition").

Please let me know if any further information and/or experiments would help.

[Daniel Marjamäki]

hmm.. I agree it is a bug but I think it's not super-critical. The condition is redundant / never true. So instead of the 'either the condition is redundant..' you should get a 'condition is always true' message.

Apparently Cppcheck does not understand that foo.index is always 0. I think https://trac.cppcheck.net/ticket/8121 is about this improvement.

[Michał Kępień]

While the warning has some merit for the sample program (because the condition is indeed redundant as it can never be satisfied), it is confusing for similar code using post-incrementation:

struct s foo = { 0 };
foo.array[foo.index++] = 1;
 if (foo.index == 1) {
     ...
 }

In this case, neither the condition is redundant nor the array is accessed out of bounds.

[Daniel Marjamäki]

yes this one is a critical FP. The warning is just wrong. Thanks!
I have created https://trac.cppcheck.net/ticket/10284