Possible regression in checking invalidFunctionArg

[helpfulchicken]

In the following example, cppcheck (1.9) reports a warning for possible invalid value, which seems justified, but cppcheck 2.5 and the current latest 2.6 dev (#737b619) do not produce this warning.

void foo(char *foo1,char *foo2)
{
    unsigned int data_len = getnum();
    if (data_len > 0)
    {
        memcpy(foo1, foo2, data_len - 2);
    }
}

Running with cppcheck using enable=all:

cppcheck --enable=all minimalMemCpyError.cpp

Result with cppcheck version 1.90:

Checking minimalMemCpyError.cpp ...
minimalMemCpyError.cpp:6:37: warning: Either the condition 'data_len>0' is redundant or memcpy() argument nr 3 can have invalid value. The value is -1 but the valid values are '0:'. [invalidFunctionArg]
        memcpy(foo1, foo2, data_len - 2);
                                    ^
minimalMemCpyError.cpp:4:18: note: Assuming that condition 'data_len>0' is not redundant
    if (data_len > 0)
                 ^
minimalMemCpyError.cpp:6:37: note: Invalid argument
        memcpy(foo1, foo2, data_len - 2);
                                    ^
minimalMemCpyError.cpp:1:0: style: The function 'foo' is never used. [unusedFunction]

^

And with cppcheck version 2.5:

minimalMemCpyError.cpp:1:0: style: The function 'foo' is never used. [unusedFunction]

^

Is this a true regression? I'm not sure which version changed this behaviour, but it seems a shame to no longer be told that I might be being dumb in this particular flavour.

[helpfulchicken]

I've checked with the repo tag for 2.4.1, and that version does report this invalidFunctionArg. So if it's some sort of regression, it's after that point.

[helpfulchicken]

Update:

I have a better minimal example:

void foo(char *foo1,char *foo2,bool flag)
{
    unsigned int data_len = getnum();
    if (data_len > 0)
    {
        if (flag) // this second condition stumps versions after 1.90
        {
            memcpy(foo1, foo2, data_len - 2);
        }
    }
}

For the above example, cppcheck 1.90 detects and reports the data_len >0 vs data_len -2 problem, but anything after 1.90 fails to find it if any simple condition is added between the data_len > 0 and its use.

Without the second condition, the problem is detected still 2.4.1 but missed in 2.5.

[Daniel Marjamäki]

I agree this is unfortunate. I am not sure how to fix this.

there has been some fixes for unsigned value. see i.e. https://trac.cppcheck.net/ticket/10298 .. if data_len is 1 then data_len - 2 will be a large positive value and that is within the configured bounds.

it is very suspicious to have an overflow in that calculation. Cppcheck must not warn about unsigned overflows in general because it is defined behavior there will be a wrap-around and this is often used intentionally.

[Daniel Marjamäki]

I created ticket https://trac.cppcheck.net/ticket/10384

[helpfulchicken]

Thanks for creating the ticket Daniel.

I've done a little more testing, and found that for repo tag 2.5 of cppcheck, if the length parameter given to memcpy is signed, then it catches the problem. However, if the length parameter is size_t (an unsigned type), the potential problem is missed.

1a) Catches possible negative argument:

void foo(char *foo1,char *foo2,bool flag, int data_len)
{
    if (data_len > 0)
    {
        memcpy(foo1, foo2, data_len - 2);
    }
}

Checking minimalMemCpyError.cpp ...
minimalMemCpyError.cpp:5:37: warning: Either the condition 'data_len>0' is redundant or memcpy() argument nr 3 can have invalid value. The value is -1 but the valid values are '0:'. [invalidFunctionArg]
        memcpy(foo1, foo2, data_len - 2);
                                    ^
minimalMemCpyError.cpp:3:18: note: Assuming that condition 'data_len>0' is not redundant
    if (data_len > 0)
                 ^
minimalMemCpyError.cpp:5:37: note: Invalid argument
        memcpy(foo1, foo2, data_len - 2);

1b) Misses suspicious overflow in unsigned type size_t

void foo(char *foo1,char *foo2,bool flag, size_t data_len)
{
    if (data_len > 0)
    {
        memcpy(foo1, foo2, data_len - 2);
    }
}

2a) Misses negative argument, hidden by simple if-condition

void foo(char *foo1,char *foo2,bool flag, int data_len)
{
    if (data_len > 0)
    {
        if (flag)
        {
            memcpy(foo1, foo2, data_len - 2);
        }
    }
}

2b) Proper behaviour - sees negative argument if condition is combined

void foo(char *foo1,char *foo2,bool flag, int data_len)
{
    if ((data_len > 0) && flag)
    {
        memcpy(foo1, foo2, data_len - 2);
    }
}

Checking minimalMemCpyError.cpp ...
minimalMemCpyError.cpp:5:37: warning: Either the condition 'data_len>0' is redundant or memcpy() argument nr 3 can have invalid value. The value is -1 but the valid values are '0:'. [invalidFunctionArg]
        memcpy(foo1, foo2, data_len - 2);
                                    ^
minimalMemCpyError.cpp:3:19: note: Assuming that condition 'data_len>0' is not redundant
    if ((data_len > 0) && flag)
                  ^
minimalMemCpyError.cpp:5:37: note: Invalid argument
        memcpy(foo1, foo2, data_len - 2);

I think there may be two things of note here.
1) suspicious overflow
2) separate condition hides potential invalid argument