Cppcheck regression?

[x-flow]

Cppcheck version 1.84 flagged this code:

void f() {
char* buf = malloc(8);
int i = 0;
for (i = 0; i < 10; i++) {
buf[i] = 's';
}
}

Summary: Array 'buf[8]' accessed at index 9, which is out of bounds.

While the master doesn't. Was there some change?

[versat]

Ouch, indeed.
I am also missing a memleak error because buf is not freed.
And if I add an explicit out of bounds array access there is also no warning:

#include <stdlib.h>
void f() {
    char* buf = malloc(8);
    int i = 0;
    for (i = 0; i < 10; i++) {
        buf[i] = 's';
    }
    buf[9] = 's';
}

If the loop is removed then I get all expected warnings with Cppcheck 1.90 dev (head).
There was a major rework of CheckBufferOverrun (see for example https://github.com/danmar/cppcheck/commit/729f57d8f1d3a4541cf9ae462b06094455ee5fb8), maybe this is a regression because of this.

[x-flow]

You are correct. Version 1.84 would flag 3 errors:

[D:/Temp/test.c:6] (error) Array 'buf[8]' accessed at index 9, which is out of bounds. [arrayIndexOutOfBounds]
[D:/Temp/test.c:8] (error) Array 'buf[8]' accessed at index 9, which is out of bounds. [arrayIndexOutOfBounds]
[D:/Temp/test.c:9] (error) Memory leak: buf [memleak]

[x-flow]

The unit tests don't seem to cover this case so I guess it can break.

[versat]

Yes, some tests were intentionally (temporarily) removed if I remember it correctly. Not sure.
As far as I see there should generally be no problem to detect this. Something must go wrong when checking the for loop, so no other errors are found any longer.

Anyway, we should create a ticket for that issue. Do you have a Trac account to create a ticket or shall I create one?

[x-flow]

I don't have a Trac account. If you could create a ticket for me that would be great.

[versat]

I have created a ticket: https://trac.cppcheck.net/ticket/9502
Thanks for reporting this.

[Daniel Marjamäki]

my first reaction was that there must be a problem with the valueflow. but that is fine!

Something must go wrong when checking the for loop, so no other errors are found any longer.

as far as i know there should be no special handling of loops in the checker.

I don't know how this can happen! A ticket is good i hope we can fix this one before the release.

[x-flow]

Is value flow correct?

Value flow using "while" loop:
Line 7
buf possible size=8
i possible {0,<9,!>10}
's' always 115

Value flow using "for" loop:
Line 7
i possible {0,9}
's' always 115

It seems to be missing: buf possible size=8.

[x-flow]

For the array out of bounds I suspect it is the malloc. For this code, the master does report 2 arrayIndexOutOfBounds errors:

void f() {
    //char* buf = malloc(8);
    char buf[8];
    int i = 0;
    for (i = 0; i < 10; i++) {
        buf[i] = 's';
    }
    buf[9] = 's';
}

[x-flow]

With while loop neither version catches the buffer over-run:

include <stdlib.h>
void f() {
    char* buf = malloc(8);
    int i = 0;
    while (i < 20)
    {
        buf[i] = 's';
        i++;
    }
    free(buf);
}

[versat]

For the initial example the function getBufferSizeValue() called at https://github.com/danmar/cppcheck/blob/91dd8d3b6f055e5f98bc32d5e57a37a9420b966b/lib/checkbufferoverrun.cpp#L210 does return a nullptr for buf. Not sure if that could be the issue. I have no time to analyse this further now.

[x-flow]

Yes, I saw that. It returns null pointer for buf. Even like this it returns null pointer:

#include <stdlib.h>
void f() {
    char* buf = malloc(8);   
    int i = 0;

    for (i = 0; i < 10; i++) {
        buf[20] = 's';
    }

    buf[9] = 0;
}

[versat]

I further debugged it and to me it looks like it is tried to set the buffer size in valueFlowDynamicBufferSize() for the malloced buffer buf. But that does not work.
valueFlowDynamicBufferSize() is called and finds out the correct size and that malloc is used. Everything looks fine.
It then calls valueFlowForwardVariable to assign the buffer size to the buffer variable I guess. But valueFlowForwardVariable() does not do anything with the given data as far as I see. To me it looks like such a case is not handled there.

[x-flow]

Yes, I looked into the issue stepping from valueFlowDynamicBufferSize but still don't understand what the code is doing at this point. It seems to work completely different if it is a for loop or a while loop.

Another thing is that the buffer overrun in "while" case is just a warning when arrayIndexOutOfBoundsCond, in the "for" case it is arrayIndexOutOfBounds.

[Daniel Marjamäki]

It seems to work completely different if it is a for loop or a while loop.

We do not want to analyze the loop body over and over. The loop body might be executed millions of times. Even if the cpu time is ignored it could lead to a lot of memory usage also. The goal is to determine the possible values for the loop counter and then "analyze" the loop body once.

For trivial for loops we can easily determine the loop counter values by determining that the loop counter is unmodified in the loop body and then looking at the for loop head.

For a while loop it is more complex to determine the possible loop counter values. The loop body must be analyzed.

[x-flow]

Makes sense.
Any ideas why this might be happening though?

[Daniel Marjamäki]

Your while loop is pretty simple, we need to add some extra code. I think it would be good to have that

[Daniel Marjamäki]

in lib/valueflow.cpp we have a function valueFlowForLoop but we do not have a valueFlowWhileLoop. It would make sense to add that.

[x-flow]

That would cover the do ... while case as well?
Also I downloaded 1.84 again and tested. It catches the issue as mentioned and the value flow is the same as master running with debug flag.

[Daniel Marjamäki]

Not sure. Maybe we should add valueFlowDoWhile also. I don't have an opinion at the moment.

if that catched the issue before it's because we had data flow analysis in checkbufferoverrun before. Nowadays we try to perform data flow analysis properly in one central place instead of having duplicated data flow analysis in each check.

[Daniel Marjamäki]

x-flow; I wonder if you would be willing to write a small test suite with short code examples that have buffer overflows.. and if you want other interesting bugs too. Doesn't have to be "complete". It's enough with 5-10 test cases, but if you like feel free to probide more :-)
Personally I think it's most interesting if the test cases are "varied" and short.

If I write a test suite I would write similar test cases as we have in our test suite. I only have 1 head. And I believe that the same head that made our old test cases is not good at inventing new ones.

[x-flow]

Sure. Here are some in attachment.
They all have buffer overruns and memory leaks. The buffer overruns are also very different to test cppcheck capability to deal with different code. I tried to keep them simple as possible.

[Daniel Marjamäki]

Thanks!

I added the attached files in the cppcheck repo with this commit: https://github.com/danmar/cppcheck/commit/aee9519d216a4102bdfd6611ae60af219a6856eb

Please feel free to contribute more tests or tweak these files.. I consider you the owner of these files so feel free to do anything you like.. I'd recommend that you open a github pull request then. if you can't open a pull request for some reason then well I can commit it for you.