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

Fix issue 9207: Not detected 'always true' and unreachable code

This commit is contained in:
Paul Fultz II 2019-08-08 07:46:47 +02:00 committed by Daniel Marjamäki
parent 2df7ce21bf
commit bd02ca5ccb
8 changed files with 228 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
lib/astutils.cpp
View File

@ -788,7 +788,23 @@ bool isUniqueExpression(const Token* tok)
return isUniqueExpression(tok->astOperand2());
}
bool isReturnScope(const Token * const endToken)
static bool isEscaped(const Token* tok, bool functionsScope)
{
if (functionsScope)
return Token::simpleMatch(tok, "throw");
else
return Token::Match(tok, "return|throw");
}
static bool isEscapedOrJump(const Token* tok, bool functionsScope)
{
if (functionsScope)
return Token::simpleMatch(tok, "throw");
else
return Token::Match(tok, "return|goto|throw|continue|break");
}
bool isReturnScope(const Token * const endToken, const Settings * settings, bool functionScope)
{
if (!endToken || endToken->str() != "}")
return false;
@ -801,28 +817,39 @@ bool isReturnScope(const Token * const endToken)
if (Token::simpleMatch(prev, "}")) {
if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {"))
return isReturnScope(prev) && isReturnScope(prev->link()->tokAt(-2));
return isReturnScope(prev, settings, functionScope) && isReturnScope(prev->link()->tokAt(-2), settings, functionScope);
if (Token::simpleMatch(prev->link()->previous(), ") {") &&
Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") &&
!Token::findsimplematch(prev->link(), "break", prev)) {
return true;
}
if (Token::Match(prev->link()->astTop(), "return|throw"))
if (isEscaped(prev->link()->astTop(), functionScope))
return true;
if (Token::Match(prev->link()->previous(), "[;{}] {"))
return isReturnScope(prev);
return isReturnScope(prev, settings, functionScope);
} else if (Token::simpleMatch(prev, ";")) {
// noreturn function
if (Token::simpleMatch(prev->previous(), ") ;") && Token::Match(prev->linkAt(-1)->tokAt(-2), "[;{}] %name% ("))
return true;
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 (settings) {
if (settings->library.isnoreturn(ftok))
return true;
}
return false;
}
if (Token::simpleMatch(prev->previous(), ") ;") && prev->previous()->link() &&
Token::Match(prev->previous()->link()->astTop(), "return|throw"))
isEscaped(prev->previous()->link()->astTop(), functionScope))
return true;
if (Token::Match(prev->previous()->astTop(), "return|throw"))
if (isEscaped(prev->previous()->astTop(), functionScope))
return true;
// return/goto statement
prev = prev->previous();
while (prev && !Token::Match(prev, ";|{|}|return|goto|throw|continue|break"))
while (prev && !Token::Match(prev, ";|{|}") && !isEscapedOrJump(prev, functionScope))
prev = prev->previous();
return prev && prev->isName();
}

2
lib/astutils.h
View File

@ -109,7 +109,7 @@ bool isWithoutSideEffects(bool cpp, const Token* tok);
bool isUniqueExpression(const Token* tok);
/** Is scope a return scope (scope will unconditionally return) */
bool isReturnScope(const Token *endToken);
bool isReturnScope(const Token *endToken, const Settings * settings = nullptr, bool functionScope=false);
/** Is variable changed by function call?
* In case the answer of the question is inconclusive, e.g. because the function declaration is not known

13
lib/symboldatabase.cpp
View File

@ -63,6 +63,7 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti
createSymbolDatabaseSetFunctionPointers(true);
createSymbolDatabaseSetTypePointers();
createSymbolDatabaseEnums();
createSymbolDatabaseEscapeFunctions();
createSymbolDatabaseIncompleteVars();
}
@ -1380,6 +1381,18 @@ void SymbolDatabase::createSymbolDatabaseIncompleteVars()
}
}
void SymbolDatabase::createSymbolDatabaseEscapeFunctions()
{
for (Scope & scope : scopeList) {
if (scope.type != Scope::eFunction)
continue;
Function * function = scope.function;
if (!function)
continue;
function->isEscapeFunction(isReturnScope(scope.bodyEnd, mSettings, true));
}
}
void SymbolDatabase::setArrayDimensionsUsingValueFlow()
{
// set all unknown array dimensions

9
lib/symboldatabase.h
View File

@ -686,6 +686,7 @@ class CPPCHECKLIB Function {
fIsVariadic = (1 << 19), ///< @brief is variadic
fIsVolatile = (1 << 20), ///< @brief is volatile
fHasTrailingReturnType = (1 << 21), ///< @brief has trailing return type
fIsEscapeFunction = (1 << 22), ///< @brief Function throws or exits
};
/**
@ -834,11 +835,16 @@ public:
bool hasTrailingReturnType() const {
return getFlag(fHasTrailingReturnType);
}
void hasBody(bool state) {
setFlag(fHasBody, state);
}
bool isEscapeFunction() const {
return getFlag(fIsEscapeFunction);
}
void isEscapeFunction(bool state) {
setFlag(fIsEscapeFunction, state);
}
bool isSafe(const Settings *settings) const;
const Token *tokenDef; ///< function name token in class definition
@ -1269,6 +1275,7 @@ private:
void createSymbolDatabaseSetVariablePointers();
void createSymbolDatabaseSetTypePointers();
void createSymbolDatabaseEnums();
void createSymbolDatabaseEscapeFunctions();
void createSymbolDatabaseIncompleteVars();
void addClassFunction(Scope **scope, const Token **tok, const Token *argStart);

78
lib/valueflow.cpp
View File

@ -1998,7 +1998,7 @@ static bool valueFlowForward(Token * const startToken,
++indentlevel;
else if (indentlevel >= 0 && tok2->str() == "}") {
--indentlevel;
if (indentlevel <= 0 && isReturnScope(tok2) && Token::Match(tok2->link()->previous(), "else|) {")) {
if (indentlevel <= 0 && isReturnScope(tok2, settings) && Token::Match(tok2->link()->previous(), "else|) {")) {
const Token *condition = tok2->link();
const bool iselse = Token::simpleMatch(condition->tokAt(-2), "} else {");
if (iselse)
@ -2039,7 +2039,7 @@ static bool valueFlowForward(Token * const startToken,
return true;
} else if (indentlevel <= 0 &&
Token::simpleMatch(tok2->link()->previous(), "else {") &&
!isReturnScope(tok2->link()->tokAt(-2)) &&
!isReturnScope(tok2->link()->tokAt(-2), settings) &&
isVariableChanged(tok2->link(), tok2, varid, var->isGlobal(), settings, tokenlist->isCPP())) {
changeKnownToPossible(values);
}
@ -2149,9 +2149,7 @@ static bool valueFlowForward(Token * const startToken,
truevalues.push_back(v);
else if (!conditionIsFalse(condTok, programMemory))
truevalues.push_back(v);
if (condAlwaysFalse)
falsevalues.push_back(v);
else if (conditionIsFalse(condTok, programMemory))
if (conditionIsFalse(condTok, programMemory))
falsevalues.push_back(v);
else if (!subFunction && !conditionIsTrue(condTok, programMemory))
falsevalues.push_back(v);
@ -2179,7 +2177,7 @@ static bool valueFlowForward(Token * const startToken,
// goto '}'
tok2 = startToken1->link();
if (isReturnScope(tok2) || !vfresult) {
if (isReturnScope(tok2, settings) || !vfresult) {
if (condAlwaysTrue)
return false;
removeValues(values, truevalues);
@ -2207,7 +2205,7 @@ static bool valueFlowForward(Token * const startToken,
// goto '}'
tok2 = startTokenElse->link();
if (isReturnScope(tok2) || !vfresult) {
if (isReturnScope(tok2, settings) || !vfresult) {
if (condAlwaysFalse)
return false;
removeValues(values, falsevalues);
@ -2283,7 +2281,7 @@ static bool valueFlowForward(Token * const startToken,
}
// stop after conditional return scopes that are executed
if (isReturnScope(end)) {
if (isReturnScope(end, settings)) {
std::list<ValueFlow::Value>::iterator it;
for (it = values.begin(); it != values.end();) {
if (conditionIsTrue(tok2->next()->astOperand2(), getProgramMemory(tok2, varid, *it)))
@ -3722,6 +3720,35 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
}
}
static void valueFlowSetConditionToKnown(const Token* tok, std::list<ValueFlow::Value>& values, bool then)
{
if (values.size() != 1U)
return;
if (values.front().isKnown())
return;
if (then && !Token::Match(tok, "==|!|("))
return;
if (!then && !Token::Match(tok, "!=|%var%|("))
return;
const char * op = "||";
if (then)
op = "&&";
const Token* parent = tok->astParent();
while (parent && parent->str() == op)
parent = parent->astParent();
if (parent && parent->str() == "(")
values.front().setKnown();
}
static bool isBreakScope(const Token* const endToken)
{
if (!Token::simpleMatch(endToken, "}"))
return false;
if (!Token::simpleMatch(endToken->link(), "{"))
return false;
return Token::findmatch(endToken->link(), "break|goto", endToken);
}
struct ValueFlowConditionHandler {
struct Condition {
const Token *vartok;
@ -3835,6 +3862,10 @@ struct ValueFlowConditionHandler {
tok2 = parent;
}
}
if (cond.true_values != cond.false_values) {
check_if = true;
check_else = true;
}
// determine startToken(s)
if (check_if && Token::simpleMatch(top->link(), ") {"))
@ -3849,13 +3880,7 @@ struct ValueFlowConditionHandler {
if (!startToken)
continue;
std::list<ValueFlow::Value> &values = (i == 0 ? cond.true_values : cond.false_values);
if (values.size() == 1U && Token::Match(tok, "==|!|(")) {
const Token *parent = tok->astParent();
while (parent && parent->str() == "&&")
parent = parent->astParent();
if (parent && parent->str() == "(")
values.front().setKnown();
}
valueFlowSetConditionToKnown(tok, values, i == 0);
bool changed = forward(startTokens[i], startTokens[i]->link(), var, values, true);
values.front().setPossible();
@ -3867,7 +3892,6 @@ struct ValueFlowConditionHandler {
startTokens[i]->link(),
"valueFlowAfterCondition: " + var->name() + " is changed in conditional block");
bail = true;
break;
}
}
if (bail)
@ -3883,7 +3907,9 @@ struct ValueFlowConditionHandler {
continue;
}
const bool dead_if = isReturnScope(after);
bool dead_if = isReturnScope(after, settings) ||
(tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (") &&
!isBreakScope(after));
bool dead_else = false;
if (Token::simpleMatch(after, "} else {")) {
@ -3893,7 +3919,7 @@ struct ValueFlowConditionHandler {
bailout(tokenlist, errorLogger, after, "possible noreturn scope");
continue;
}
dead_else = isReturnScope(after);
dead_else = isReturnScope(after, settings);
}
std::list<ValueFlow::Value> *values = nullptr;
@ -3903,6 +3929,10 @@ struct ValueFlowConditionHandler {
values = &cond.false_values;
if (values) {
if ((dead_if || dead_else) && !Token::Match(tok->astParent(), "&&|&")) {
valueFlowSetConditionToKnown(tok, *values, true);
valueFlowSetConditionToKnown(tok, *values, false);
}
// TODO: constValue could be true if there are no assignments in the conditional blocks and
// perhaps if there are no && and no || in the condition
bool constValue = false;
@ -3980,14 +4010,24 @@ static void valueFlowAfterCondition(TokenList *tokenlist,
vartok = vartok->astOperand1();
if (!vartok->isName())
return cond;
if (astIsPointer(vartok) && true_value.intvalue == 0) {
if (Token::simpleMatch(tok, "=="))
false_value.intvalue = 1;
if (Token::simpleMatch(tok, "!="))
true_value.intvalue = 1;
}
cond.true_values.push_back(true_value);
cond.false_values.push_back(false_value);
cond.vartok = vartok;
return cond;
}
long long falseIntValue = 0LL;
if (tok->str() == "!") {
vartok = tok->astOperand1();
if (astIsPointer(vartok))
falseIntValue = 1LL;
} else if (tok->isName() && (Token::Match(tok->astParent(), "%oror%|&&") ||
Token::Match(tok->tokAt(-2), "if|while ( %var% [)=]"))) {
@ -3997,7 +4037,7 @@ static void valueFlowAfterCondition(TokenList *tokenlist,
if (!vartok || !vartok->isName())
return cond;
cond.true_values.emplace_back(tok, 0LL);
cond.false_values.emplace_back(tok, 0LL);
cond.false_values.emplace_back(tok, falseIntValue);
cond.vartok = vartok;
return cond;

65
test/testcondition.cpp
View File

@ -525,13 +525,13 @@ private:
" if (a) { b = 1; }\n"
" else { if (a) { b = 2; } }\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str());
check("void f(int a, int &b) {\n"
" if (a) { b = 1; }\n"
" else { if (a) { b = 2; } }\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str());
check("void f(int a, int &b) {\n"
" if (a == 1) { b = 1; }\n"
@ -701,9 +701,9 @@ private:
" if (x) {}\n"
" else if (!x) {}\n"
"}");
ASSERT_EQUALS("test.cpp:3:style:Expression is always true because 'else if' condition is opposite to previous condition at line 2.\n"
"test.cpp:2:note:first condition\n"
"test.cpp:3:note:else if condition is opposite to first condition\n", errout.str());
ASSERT_EQUALS("test.cpp:3:style:Condition '!x' is always true\n"
"test.cpp:2:note:condition 'x'\n"
"test.cpp:3:note:Condition '!x' is always true\n", errout.str());
check("void f(int x) {\n"
" int y = x;\n"
@ -3120,6 +3120,61 @@ private:
" return ret;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("int f(void *handle) {\n"
" if (!handle) return 0;\n"
" if (handle) return 1;\n"
" else return 0;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'handle' is always true\n", errout.str());
check("int f(void *handle) {\n"
" if (handle == 0) return 0;\n"
" if (handle) return 1;\n"
" else return 0;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'handle' is always true\n", errout.str());
check("int f(void *handle) {\n"
" if (handle != 0) return 0;\n"
" if (handle) return 1;\n"
" else return 0;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'handle' is always false\n", errout.str());
check("void f(void* x, void* y) {\n"
" if (x == nullptr && y == nullptr)\n"
" return;\n"
" if (x == nullptr || y == nullptr)\n"
" return;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("void* g();\n"
"void f(void* a, void* b) {\n"
" while (a) {\n"
" a = g();\n"
" if (a == b)\n"
" break;\n"
" }\n"
" if (a) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("void* g();\n"
"void f(void* a, void* b) {\n"
" while (a) {\n"
" a = g();\n"
" }\n"
" if (a) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:6]: (style) Condition 'a' is always false\n", errout.str());
check("void f(int * x, bool b) {\n"
" if (!x && b) {}\n"
" else if (x) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void multiConditionAlwaysTrue() {

24
test/testnullpointer.cpp
View File

@ -72,6 +72,8 @@ private:
TEST_CASE(nullpointer30); // #6392
TEST_CASE(nullpointer31); // #8482
TEST_CASE(nullpointer32); // #8460
TEST_CASE(nullpointer33);
TEST_CASE(nullpointer34);
TEST_CASE(nullpointer_addressOf); // address of
TEST_CASE(nullpointerSwitch); // #2626
TEST_CASE(nullpointer_cast); // #4692
@ -1389,6 +1391,28 @@ private:
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:6]: (warning) Either the condition 'ptr' is redundant or there is possible null pointer dereference: p1.\n", errout.str());
}
void nullpointer33() {
check("void f(int * x) {\n"
" if (x != nullptr)\n"
" *x = 2;\n"
" else\n"
" *x = 3;\n"
"}\n", true);
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:5]: (warning) Either the condition 'x!=nullptr' is redundant or there is possible null pointer dereference: x.\n", errout.str());
}
void nullpointer34() {
check("void g() {\n"
" throw "";\n"
"}\n"
"bool f(int * x) {\n"
" if (x) *x += 1;\n"
" if (!x) g();\n"
" return *x;\n"
"}\n", true);
ASSERT_EQUALS("", errout.str());
}
void nullpointer_addressOf() { // address of
check("void f() {\n"
" struct X *x = 0;\n"

28
test/testvalueflow.cpp
View File

@ -2289,6 +2289,16 @@ private:
" }\n"
"}";
ASSERT_EQUALS(false, testValueOfX(code, 6U, 0));
code = "int* g();\n"
"int f() {\n"
" int * x;\n"
" x = g();\n"
" if (x) { printf(\"\"); }\n"
" return *x;\n"
"}\n";
ASSERT_EQUALS(false, testValueOfXKnown(code, 6U, 0));
ASSERT_EQUALS(true, testValueOfX(code, 6U, 0));
}
void valueFlowAfterConditionSeveralNot() {
@ -2858,7 +2868,7 @@ private:
" int x;\n"
" for (x = 0; x < 5; x++) {}\n"
" if (x == 5) {\n"
" panic();\n"
" abort();\n"
" }\n"
" a = x;\n" // <- x can't be 5
"}";
@ -3545,7 +3555,7 @@ private:
" int c;\n"
" if (d)\n"
" c = 0;\n"
" else if (!d)\n"
" else if (e)\n"
" c = 0;\n"
" c++;\n"
"}\n";
@ -3555,6 +3565,20 @@ private:
ASSERT_EQUALS(true, values.front().isPossible() || values.back().isPossible());
ASSERT_EQUALS(true, values.front().intvalue == 0 || values.back().intvalue == 0);
code = "void b(bool d, bool e) {\n"
" int c;\n"
" if (d)\n"
" c = 0;\n"
" else if (!d)\n"
" c = 0;\n"
" c++;\n"
"}\n";
values = tokenValues(code, "c ++ ; }");
ASSERT_EQUALS(true, values.size() == 1);
// TODO: Value should be known
ASSERT_EQUALS(true, values.back().isPossible());
ASSERT_EQUALS(true, values.back().intvalue == 0);
code = "void f() {\n" // sqlite
" int szHdr;\n"
" idx = (A<0x80) ? (szHdr = 0) : dostuff(A, (int *)&(szHdr));\n"