Fix crashes in valueflow

[Ken-Patrick]

http://cppcheck1.osuosl.org:8000/crash.html

For instance in http://cppcheck1.osuosl.org:8000/styx

==19651==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x556f21abc3df bp 0x7ffc140d2720 sp 0x7ffc140d2710 T0)
==19651==The signal is caused by a READ memory access.
==19651==Hint: address points to the zero page.
    #0 0x556f21abc3de in Variable::isGlobal() const ../lib/symboldatabase.h:342
    #1 0x556f221f801a in valueFlowForwardVariable ../lib/valueflow.cpp:2471
    #2 0x556f22208130 in valueFlowForward ../lib/valueflow.cpp:3204
    #3 0x556f221e9e14 in valueFlowReverse ../lib/valueflow.cpp:1892
    #4 0x556f221f1a43 in valueFlowBeforeCondition ../lib/valueflow.cpp:2200
    #5 0x556f2223dbb5 in ValueFlow::setValues(TokenList*, SymbolDatabase*, ErrorLogger*, Settings const*) ../lib/valueflow.cpp:6521
    #6 0x556f220e5991 in Tokenizer::simplifyTokens1(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ../lib/tokenize.cpp:2342
    #7 0x556f21d8d066 in CppCheck::checkFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::istream&) ../lib/cppcheck.cpp:508
    #8 0x556f21d84cd3 in CppCheck::check(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ../lib/cppcheck.cpp:192
    #9 0x556f21a28796 in CppCheckExecutor::check_internal(CppCheck&, int, char const* const*) ../cli/cppcheckexecutor.cpp:884
    #10 0x556f21a24be8 in CppCheckExecutor::check(int, char const* const*) ../cli/cppcheckexecutor.cpp:198
    #11 0x556f22313063 in main ../cli/main.cpp:95

[amai2012]

A test would be nice.
Do you think that is the root cause? Or is cppcheck misunderstanding the code which later on leads to this crash?

[danmar]

Spontaneously... I don't remember any valid reason that Token::variable() is NULL even though Token::varId is non-zero. Is there some debug warning or something about that? Maybe it's better to create the variable or to fix the Tokenizer::setVarId() somehow..

[pfultz2]

I don't remember any valid reason that Token::variable() is NULL even though Token::varId is non-zero.

This happens when the source code is incomplete, like this:

void f(struct mailbox *mb)
{
	if (mb->x == nullptr) {}
}

Cppcheck will assign a varid to x, but there wont be a Variable object created because the definition of the mailbox struct is missing.

[amai2012]

I've reduced one example so that's still causing a SIGSEGV:

void SlopeFloor::setAttr(const Value &val) {
    int slope = val;
    if (slope >= -1)
        state = slope;
}

[danmar]

It seems to me that a Variable could be created even if we don't have exact type info. The typeStart/typeEnd/access/etc properties can be set without a type. @IOBYTE what do you think?

An attribute can be added that explains the variable has unknown type.

void f(struct mailbox *mb)
{
    if (mb->x == nullptr) {}
}

We know that for mb, isClass should be true and that isArray should be false. But the type() and typeScope() would be nullptr.

void SlopeFloor::setAttr(const Value &val) {
    int slope = val;
    if (slope >= -1)
        state = slope;
}

I would suggest that we set the isReference() attribute. For isClass we can only guess. The "safe" guess might be to set isClass to true because that means the type might have various unseen sideeffects. Alternatively we only set isClass/isArray if we can actually determine that it's definitely a class or array.

[amai2012]

It seems to me that a Variable could be created even if we don't have exact type info.

I assume that could be a change having a major impact. Maybe we should accept the PR first to get daca@home more stable again.

[IOBYTE]

I think the variable exists but can't be found. The example has a class function without a class defined which means the symbol database is incomplete. Looking up the variable through the class will fail because there is no class but you should still be able to look up the variable though the function.

[IOBYTE]

A variable with an unknown type is common. It will just have a null type pointer.

[danmar]

I think the variable exists but can't be found. The example has a class function without a class defined which means the symbol database is incomplete. Looking up the variable through the class will fail because there is no class but you should still be able to look up the variable though the function.

ok... if we see a class function implementation but not the class definition... maybe we should stop the analysis and ask the user to fix the include paths. As far as I remember there was tricky problems with that - I have the feeling there is some open ticket.

[IOBYTE]

I looked into this a little deeper. There is a function scope but there is no function because it would be contained in the missing class. Creating a fake class or creating the function in the global scope would be wrong. I think it would cause more problems then it solves. This has been discussed before somewhere.

We already have a debug warning for this:

2236.cpp:1:18: debug: Executable scope 'setAttr' with unknown function. [symbolDatabaseWarning]
void SlopeFloor::setAttr(const Value &val) {
                 ^

although it doesn't really explain the real problem well.

[IOBYTE]

I think this patch is OK but we can now add a test. Hopefully this patch only introduces false negatives. The existing debug warning can be turned into a regular warning explaining that there are probably missing includes and they need to fix that if they want better results.

[Ken-Patrick]

I have added a test case.

[amai2012]

ok... if we see a class function implementation but not the class definition... maybe we should stop the analysis and ask the user to fix the include paths. As far as I remember there was tricky problems with that - I have the feeling there is some open ticket.

IMHO it's a very helpful detail of cppcheck that it's to run on incomplete code. At least if one is aware of the risks and shortcoming in that scenario...
So I'd be in favor of accepting this PR.

[slacka]

This regression was caused by commit 9978038
Author: @pfultz2 Paul Fultz II
Forward values after assignment in valueFlowReverse (#2226)

See #9393 for another simple reproducer

[danmar]

I don't think PR has the proper fix. There should be a variable even if the precise type is not known.

IMHO it's a very helpful detail of cppcheck that it's to run on incomplete code. At least if one is aware of the risks and shortcoming in that scenario...

Yes of course! But if you have such code in a source file:

void Foo::dostuff() {}

Then the Foo class declaration is in the corresponding header file. I'd assume it's not a big deal to include that. This is not about including some system header it's about including your header that is in your project.

Handling this without having the class declaration.. I have the feeling it's not as easy as it sounds. I have the feeling we had some ticket somewhere but I can't find it. If we can handle it in a decent way it's better to analyze the code but if we really can't handle that in a way that we feel comfortable with it's better to bailout.

[danmar]

I looked into this a little deeper. There is a function scope but there is no function because it would be contained in the missing class. Creating a fake class or creating the function in the global scope would be wrong. I think it would cause more problems then it solves. This has been discussed before somewhere.

Thanks @IOBYTE! I have the feeling I also looked at this a few years ago and failed to see any good automated approach for this.

If we can make some workaround or good guess for incomplete code that is best! But sometimes it's just not possible.

[danmar]

I merge this anyway.. after all.. I think we can merge this as a temporary hack.