diff --git a/lib/astutils.cpp b/lib/astutils.cpp index d1deaef9f..30f25e0be 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -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(); } diff --git a/lib/astutils.h b/lib/astutils.h index 437411f54..847c25417 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -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 diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 1c9aa715f..953c8256f 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -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 diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 3a13d92a8..339930c35 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -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); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index fe44632d9..c7d68ec7c 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -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::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& 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 &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 *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; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 519097f0a..cbcdd83a1 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -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() { diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 7df3bc8ba..38b82b0bb 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -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" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 6e544111f..9c08227bd 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -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"