walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fix issue 8144: valueFlowBeforeCondition: struct (#2645)

This commit is contained in:
Paul Fultz II 2020-05-21 01:47:48 -05:00 committed by GitHub
parent 90550d24c4
commit 8301fa8244
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 147 additions and 135 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
lib/astutils.cpp
View File

@ -1085,6 +1085,35 @@ bool isEscapeFunction(const Token* ftok, const Library* library)
return false;
}
static bool hasReturnFunction(const Token* tok, const Library* library, const Token** unknownFunc)
{
if (!tok)
return false;
const Token* ftok = tok->str() == "(" ? tok->previous() : nullptr;
while (Token::simpleMatch(ftok, "("))
ftok = ftok->astOperand1();
if (ftok) {
const Function * function = ftok->function();
if (function) {
if (function->isEscapeFunction())
return true;
if (function->isAttributeNoreturn())
return true;
} else if (library && library->isnoreturn(ftok)) {
return true;
} else if (Token::Match(ftok, "exit|abort")) {
return true;
}
if (unknownFunc && !function && library && library->functions.count(library->getFunctionName(ftok)) == 0)
*unknownFunc = ftok;
return false;
} else if (tok->isConstOp()) {
return hasReturnFunction(tok->astOperand1(), library, unknownFunc) || hasReturnFunction(tok->astOperand2(), library, unknownFunc);
}
return false;
}
bool isReturnScope(const Token* const endToken, const Library* library, const Token** unknownFunc, bool functionScope)
{
if (!endToken || endToken->str() != "}")
@ -1110,21 +1139,12 @@ bool isReturnScope(const Token* const endToken, const Library* library, const To
if (Token::Match(prev->link()->previous(), "[;{}] {"))
return isReturnScope(prev, library, unknownFunc, functionScope);
} else if (Token::simpleMatch(prev, ";")) {
if (Token::simpleMatch(prev->previous(), ") ;") && Token::Match(prev->linkAt(-1)->tokAt(-2), "[;{}] %name% (")) {
const Token * ftok = prev->linkAt(-1)->previous();
const Function * function = ftok->function();
if (function) {
if (function->isEscapeFunction())
return true;
if (function->isAttributeNoreturn())
return true;
} else if (library && library->isnoreturn(ftok)) {
return true;
} else if (Token::Match(ftok, "exit|abort")) {
return true;
}
if (unknownFunc && !function && library && library->functions.count(library->getFunctionName(ftok)) == 0)
*unknownFunc = ftok;
if (prev->tokAt(-2) && hasReturnFunction(prev->tokAt(-2)->astTop(), library, unknownFunc))
return true;
// Unknown symbol
if (Token::Match(prev->tokAt(-2), ";|}|{ %name% ;") && prev->previous()->isIncompleteVar()) {
if (unknownFunc)
*unknownFunc = prev->previous();
return false;
}
if (Token::simpleMatch(prev->previous(), ") ;") && prev->previous()->link() &&

76
lib/checknullpointer.cpp
View File

@ -195,7 +195,13 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set
// read/write member variable
if (firstOperand && parent->originalName() == "->" && (!parent->astParent() || parent->astParent()->str() != "&")) {
if (!parent->astParent() || parent->astParent()->str() != "(" || parent->astParent() == tok->previous())
if (!parent->astParent())
return true;
if (!Token::Match(parent->astParent()->previous(), "if|while|for|switch ("))
return true;
if (!Token::Match(parent->astParent()->previous(), "%name% ("))
return true;
if (parent->astParent() == tok->previous())
return true;
unknown = true;
return false;
@ -251,73 +257,6 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set
}
void CheckNullPointer::nullPointerLinkedList()
{
if (!mSettings->isEnabled(Settings::WARNING))
return;
const SymbolDatabase* const symbolDatabase = mTokenizer->getSymbolDatabase();
// looping through items in a linked list in a inner loop.
// Here is an example:
// for (const Token *tok = tokens; tok; tok = tok->next) {
// if (tok->str() == "hello")
// tok = tok->next; // <- tok might become a null pointer!
// }
for (const Scope &forScope : symbolDatabase->scopeList) {
const Token* const tok1 = forScope.classDef;
// search for a "for" scope..
if (forScope.type != Scope::eFor || !tok1)
continue;
// is there any dereferencing occurring in the for statement
const Token* end2 = tok1->linkAt(1);
for (const Token *tok2 = tok1->tokAt(2); tok2 != end2; tok2 = tok2->next()) {
// Dereferencing a variable inside the "for" parentheses..
if (Token::Match(tok2, "%var% . %name%")) {
// Is this variable a pointer?
const Variable *var = tok2->variable();
if (!var || !var->isPointer())
continue;
// Variable id for dereferenced variable
const unsigned int varid(tok2->varId());
if (Token::Match(tok2->tokAt(-2), "%varid% ?", varid))
continue;
// Check usage of dereferenced variable in the loop..
// TODO: Move this to ValueFlow
for (const Scope *innerScope : forScope.nestedList) {
if (innerScope->type != Scope::eWhile)
continue;
// TODO: are there false negatives for "while ( %varid% ||"
if (Token::Match(innerScope->classDef->next(), "( %varid% &&|)", varid)) {
// Make sure there is a "break" or "return" inside the loop.
// Without the "break" a null pointer could be dereferenced in the
// for statement.
for (const Token *tok4 = innerScope->bodyStart; tok4; tok4 = tok4->next()) {
if (tok4 == forScope.bodyEnd) {
const ValueFlow::Value v(innerScope->classDef, 0LL);
nullPointerError(tok1, var->name(), &v, false);
break;
}
// There is a "break" or "return" inside the loop.
// TODO: there can be false negatives. There could still be
// execution paths that are not properly terminated
else if (tok4->str() == "break" || tok4->str() == "return")
break;
}
}
}
}
}
}
}
static bool isNullablePointer(const Token* tok, const Settings* settings)
{
if (!tok)
@ -374,7 +313,6 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
void CheckNullPointer::nullPointer()
{
nullPointerLinkedList();
nullPointerByDeRefAndChec();
}

6
lib/checknullpointer.h
View File

@ -137,12 +137,6 @@ private:
"- undefined null pointer arithmetic\n";
}
/**
* @brief Does one part of the check for nullPointer().
* looping through items in a linked list in a inner loop..
*/
void nullPointerLinkedList();
/**
* @brief Does one part of the check for nullPointer().
* Dereferencing a pointer and then checking if it's NULL..

8
lib/forwardanalyzer.cpp
View File

@ -407,8 +407,12 @@ struct ForwardTraversal {
} else {
if (updateTok(tok, &next) == Progress::Break)
return Progress::Break;
if (next)
tok = next;
if (next) {
if (precedes(next, end))
tok = next->previous();
else
return Progress::Break;
}
}
// Prevent infinite recursion
if (tok->next() == start)

8
lib/templatesimplifier.cpp
View File

@ -147,10 +147,10 @@ TemplateSimplifier::TokenAndName::TokenAndName(Token *token, const std::string &
if ((isFunction() || isClass()) && mNameToken->strAt(-1) == "::") {
const Token * start = mNameToken;
while (Token::Match(start->tokAt(-2), "%name% ::") ||
(Token::simpleMatch(start->tokAt(-2), "> ::") &&
start->tokAt(-2)->findOpeningBracket() &&
Token::Match(start->tokAt(-2)->findOpeningBracket()->previous(), "%name% <"))) {
while (start && (Token::Match(start->tokAt(-2), "%name% ::") ||
(Token::simpleMatch(start->tokAt(-2), "> ::") &&
start->tokAt(-2)->findOpeningBracket() &&
Token::Match(start->tokAt(-2)->findOpeningBracket()->previous(), "%name% <")))) {
if (start->strAt(-2) == ">")
start = start->tokAt(-2)->findOpeningBracket()->previous();
else

6
lib/tokenize.cpp
View File

@ -1686,6 +1686,8 @@ namespace {
void setScopeInfo(Token *tok, std::list<ScopeInfo3> *scopeInfo, bool all = false)
{
if (!tok)
return;
while (tok->str() == "}" && !scopeInfo->empty() && tok == scopeInfo->back().bodyEnd)
scopeInfo->pop_back();
if (!Token::Match(tok, "namespace|class|struct|union %name% {|:|::")) {
@ -5902,7 +5904,7 @@ bool Tokenizer::simplifyConditions()
Token::simpleMatch(tok, "|| true ||")) {
//goto '('
Token *tok2 = tok;
while (tok2->previous()) {
while (tok2 && tok2->previous()) {
if (tok2->previous()->str() == ")")
tok2 = tok2->previous()->link();
else {
@ -6466,7 +6468,7 @@ void Tokenizer::simplifyFunctionPointers()
!(Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") ) (")))
continue;
while (tok->str() != "(")
while (tok && tok->str() != "(")
tok = tok->next();
// check that the declaration ends

47
lib/valueflow.cpp
View File

@ -102,6 +102,7 @@
#include <map>
#include <set>
#include <stack>
#include <tuple>
#include <vector>
static void bailoutInternal(TokenList *tokenlist, ErrorLogger *errorLogger, const Token *tok, const std::string &what, const std::string &file, int line, const std::string &function)
@ -1838,6 +1839,29 @@ static void valueFlowReverse(TokenList *tokenlist,
}
if (tok2->str() == "}") {
const Token* condTok = getCondTokFromEnd(tok2);
// Evaluate condition of for and while loops first
if (condTok && condTok->astTop() && Token::Match(condTok->astTop()->previous(), "for|while (")) {
const Token* startTok = nullptr;
const Token* endTok = nullptr;
std::tie(startTok, endTok) = condTok->findExpressionStartEndTokens();
if (!isVariableChanged(startTok, endTok, varid, false, settings, true)) {
std::list<ValueFlow::Value> values = {val};
if (val2.condition) {
values.push_back(val2);
}
const Token *expr = Token::findmatch(tok2, "%varid%", varid);
valueFlowForward(const_cast<Token*>(startTok),
endTok,
expr,
values,
false,
false,
tokenlist,
errorLogger,
settings);
}
}
const Token *vartok = Token::findmatch(tok2->link(), "%varid%", tok2, varid);
while (Token::Match(vartok, "%name% = %num% ;") && !vartok->tokAt(2)->getValue(num))
vartok = Token::findmatch(vartok->next(), "%varid%", tok2, varid);
@ -4089,26 +4113,27 @@ struct ValueFlowConditionHandler {
// After conditional code..
if (Token::simpleMatch(top->link(), ") {")) {
Token *after = top->link()->linkAt(1);
std::string unknownFunction;
if (settings->library.isScopeNoReturn(after, &unknownFunction)) {
if (settings->debugwarnings && !unknownFunction.empty())
bailout(tokenlist, errorLogger, after, "possible noreturn scope");
continue;
}
bool dead_if = isReturnScope(after, &settings->library) ||
const Token* unknownFunction = nullptr;
bool dead_if = isReturnScope(after, &settings->library, &unknownFunction) ||
(tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (") &&
!isBreakScope(after));
bool dead_else = false;
if (!dead_if && unknownFunction) {
if (settings->debugwarnings)
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
continue;
}
if (Token::simpleMatch(after, "} else {")) {
after = after->linkAt(2);
if (Token::simpleMatch(after->tokAt(-2), ") ; }")) {
unknownFunction = nullptr;
dead_else = isReturnScope(after, &settings->library, &unknownFunction);
if (!dead_else && unknownFunction) {
if (settings->debugwarnings)
bailout(tokenlist, errorLogger, after, "possible noreturn scope");
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
continue;
}
dead_else = isReturnScope(after, &settings->library);
}
if (dead_if && dead_else)

79
test/testnullpointer.cpp
View File

@ -95,6 +95,7 @@ private:
TEST_CASE(nullpointer52);
TEST_CASE(nullpointer53); // #8005
TEST_CASE(nullpointer54); // #9573
TEST_CASE(nullpointer55); // #8144
TEST_CASE(nullpointer_addressOf); // address of
TEST_CASE(nullpointerSwitch); // #2626
TEST_CASE(nullpointer_cast); // #4692
@ -179,7 +180,7 @@ private:
" while (tok);\n"
" tok = tok->next();\n"
"}", true);
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning, inconclusive) Either the condition 'tok' is redundant or there is possible null pointer dereference: tok.\n", errout.str());
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'tok' is redundant or there is possible null pointer dereference: tok.\n", errout.str());
// #2681
{
@ -192,11 +193,8 @@ private:
" ;\n"
"}\n";
check(code, false); // inconclusive=false => no error
ASSERT_EQUALS("", errout.str());
check(code, true); // inconclusive=true => error
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:6]: (warning, inconclusive) Either the condition 'tok' is redundant or there is possible null pointer dereference: tok.\n", errout.str());
check(code);
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:6]: (warning) Either the condition 'tok' is redundant or there is possible null pointer dereference: tok.\n", errout.str());
}
check("void foo()\n"
@ -207,7 +205,7 @@ private:
" tok = tok->next();\n"
" }\n"
"}");
ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (warning) Either the condition 'while' is redundant or there is possible null pointer dereference: tok.\n", errout.str());
TODO_ASSERT_EQUALS("", "[test.cpp:5] -> [test.cpp:3]: (warning) Either the condition 'while' is redundant or there is possible null pointer dereference: tok.\n", errout.str());
check("void foo(Token &tok)\n"
"{\n"
@ -276,6 +274,18 @@ private:
" char a[2] = {0,0};\n"
"}\n", true);
ASSERT_EQUALS("", errout.str());
check("struct b {\n"
" b * c;\n"
" int i;\n"
"}\n"
"void a(b * e) {\n"
" for (b *d = e;d; d = d->c)\n"
" while (d && d->i == 0)\n"
" d = d->c;\n"
" if (!d) throw;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void nullpointer1() {
@ -553,9 +563,7 @@ private:
" if (fred) { }\n"
"}";
check(code);
ASSERT_EQUALS("", errout.str());
check(code, true);
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning, inconclusive) Either the condition 'if(fred)' is redundant or there is possible null pointer dereference: fred.\n", errout.str());
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Either the condition 'if(fred)' is redundant or there is possible null pointer dereference: fred.\n", errout.str());
}
// #3425 - false positives when there are macros
@ -949,13 +957,8 @@ private:
" abc->a();\n"
"}\n";
// inconclusive=false => no error
check(code,false);
ASSERT_EQUALS("", errout.str());
// inconclusive=true => error
check(code, true);
ASSERT_EQUALS("[test.cpp:3]: (error, inconclusive) Null pointer dereference: abc\n", errout.str());
check(code);
ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference: abc\n", errout.str());
}
check("static void foo() {\n"
@ -1082,12 +1085,12 @@ private:
"}", true);
ASSERT_EQUALS("", errout.str());
// ticket #3220: calling member function
// ticket #3220: dereferencing a null pointer is UB
check("void f() {\n"
" Fred *fred = NULL;\n"
" fred->do_something();\n"
"}");
ASSERT_EQUALS("", errout.str());
ASSERT_EQUALS("[test.cpp:3]: (error) Null pointer dereference: fred\n", errout.str());
// ticket #3570 - parsing of conditions
{
@ -1392,7 +1395,7 @@ private:
" }\n"
" }\n"
"}\n", true);
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (warning, inconclusive) Either the condition 'if(values)' is redundant or there is possible null pointer dereference: values.\n", errout.str());
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (warning) Either the condition 'if(values)' is redundant or there is possible null pointer dereference: values.\n", errout.str());
}
void nullpointer31() { // #8482
@ -1779,6 +1782,35 @@ private:
ASSERT_EQUALS("", errout.str());
}
void nullpointer55() {
check("void f(const Token* tok) {\n"
" const Token* tok3 = tok;\n"
" while (tok3->astParent() && tok3->str() == \",\")\n"
" tok3 = tok3->astParent();\n"
" if (tok3 && tok3->str() == \"(\") {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (warning) Either the condition 'if(tok3&&tok3->str()==\"(\")' is redundant or there is possible null pointer dereference: tok3.\n", errout.str());
check("void f(int* t1, int* t2) {\n"
" while (t1 && t2 &&\n"
" *t1 == *t2) {\n"
" t1 = nullptr;\n"
" t2 = nullptr;\n"
" }\n"
" if (!t1 || !t2)\n"
" return;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("bool f(int* i);\n"
"void g(int* i) {\n"
" while(f(i) && *i == 0)\n"
" i++;\n"
" if (!i) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void nullpointer_addressOf() { // address of
check("void f() {\n"
" struct X *x = 0;\n"
@ -2157,11 +2189,8 @@ private:
" fred->x();\n"
"}";
check(code, false); // non-inconclusive
ASSERT_EQUALS("", errout.str());
check(code, true); // inconclusive
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning, inconclusive) Either the condition 'fred==NULL' is redundant or there is possible null pointer dereference: fred.\n", errout.str());
check(code); // inconclusive
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning) Either the condition 'fred==NULL' is redundant or there is possible null pointer dereference: fred.\n", errout.str());
}
check("void f(char *s) {\n" // #3358

2
test/testvalueflow.cpp
View File

@ -4342,7 +4342,7 @@ private:
" abort() << 123;\n"
" ints[0] = 0;\n"
"}";
ASSERT(tokenValues(code, "ints [").empty());
ASSERT_EQUALS("", isImpossibleContainerSizeValue(tokenValues(code, "ints ["), 0));
code = "struct A {\n" // forward, nested function call, #9424
" double getMessage( std::vector<unsigned char> *message );\n"