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

Fix 11579: false negative: knownConditionTrueFalse with non-bool as bool parameter (#5349) 

This adds a new checker to check for pointer to bool conversions that
are always known. I removed the previous knownConditionTrueFalse checks
since this was too noisy.
This commit is contained in:
Paul Fultz II 2023-08-20 15:32:41 -05:00 committed by GitHub
parent a5cfa85e0d
commit 03b952d5eb
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 181 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
lib/astutils.cpp
View File

@ -696,9 +696,10 @@ std::vector<ValueType> getParentValueTypes(const Token* tok, const Settings* set
return {*tok->astParent()->astOperand1()->valueType()};
return {};
}
const Token* ftok = nullptr;
if (Token::Match(tok->astParent(), "(|{|,")) {
int argn = -1;
const Token* ftok = getTokenArgumentFunction(tok, argn);
ftok = getTokenArgumentFunction(tok, argn);
const Token* typeTok = nullptr;
if (ftok && argn >= 0) {
if (ftok->function()) {
@ -741,6 +742,9 @@ std::vector<ValueType> getParentValueTypes(const Token* tok, const Settings* set
ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, *settings);
return {std::move(vtParent)};
}
// The return type of a function is not the parent valuetype
if (Token::simpleMatch(tok->astParent(), "(") && ftok && !tok->isCast() && ftok->tokType() != Token::eType)
return {};
if (Token::Match(tok->astParent(), "return|(|{|%assign%") && parent) {
*parent = tok->astParent();
}
@ -1422,7 +1426,7 @@ static bool isForLoopIncrement(const Token* const tok)
parent->astParent()->astParent()->astOperand1()->str() == "for";
}
bool isUsedAsBool(const Token* const tok)
bool isUsedAsBool(const Token* const tok, const Settings* settings)
{
if (!tok)
return false;
@ -1435,6 +1439,15 @@ bool isUsedAsBool(const Token* const tok)
const Token* parent = tok->astParent();
if (!parent)
return false;
if (Token::simpleMatch(parent, "["))
return false;
if (parent->isUnaryOp("*"))
return false;
if (Token::simpleMatch(parent, ".")) {
if (astIsRHS(tok))
return isUsedAsBool(parent, settings);
return false;
}
if (Token::Match(parent, "&&|!|%oror%"))
return true;
if (parent->isCast())
@ -1451,7 +1464,7 @@ bool isUsedAsBool(const Token* const tok)
if (isForLoopCondition(tok))
return true;
if (!Token::Match(parent, "%cop%")) {
std::vector<ValueType> vtParents = getParentValueTypes(tok);
std::vector<ValueType> vtParents = getParentValueTypes(tok, settings);
return std::any_of(vtParents.cbegin(), vtParents.cend(), [&](const ValueType& vt) {
return vt.pointer == 0 && vt.type == ValueType::BOOL;
});

2
lib/astutils.h
View File

@ -254,7 +254,7 @@ const Token* isInLoopCondition(const Token* tok);
/**
* Is token used as boolean, that is to say cast to a bool, or used as a condition in a if/while/for
*/
CPPCHECKLIB bool isUsedAsBool(const Token * const tok);
CPPCHECKLIB bool isUsedAsBool(const Token* const tok, const Settings* settings = nullptr);
/**
* Is token passed to a function taking a bool argument

15
lib/checkcondition.cpp
View File

@ -60,7 +60,7 @@ bool CheckCondition::diag(const Token* tok, bool insert)
return false;
const Token* parent = tok->astParent();
bool hasParent = false;
while (Token::Match(parent, "&&|%oror%")) {
while (Token::Match(parent, "!|&&|%oror%")) {
if (mCondDiags.count(parent) != 0) {
hasParent = true;
break;
@ -410,6 +410,8 @@ void CheckCondition::comparison()
void CheckCondition::comparisonError(const Token *tok, const std::string &bitop, MathLib::bigint value1, const std::string &op, MathLib::bigint value2, bool result)
{
if (tok && (diag(tok) | diag(tok->astParent())))
return;
std::ostringstream expression;
expression << std::hex << "(X " << bitop << " 0x" << value1 << ") " << op << " 0x" << value2;
@ -1370,6 +1372,8 @@ void CheckCondition::checkModuloAlwaysTrueFalse()
void CheckCondition::moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal)
{
if (diag(tok))
return;
reportError(tok, Severity::warning, "moduloAlwaysTrueFalse",
"Comparison of modulo result is predetermined, because it is always less than " + maxVal + ".", CWE398, Certainty::normal);
}
@ -1466,7 +1470,7 @@ void CheckCondition::alwaysTrueFalse()
for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
if (Token::simpleMatch(tok, "<") && tok->link()) // don't write false positives when templates are used
continue;
if (!tok->hasKnownBoolValue())
if (!tok->hasKnownIntValue())
continue;
if (Token::Match(tok->previous(), "%name% (") && tok->previous()->function()) {
const Function* f = tok->previous()->function();
@ -1490,7 +1494,7 @@ void CheckCondition::alwaysTrueFalse()
else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() &&
Token::simpleMatch(parent->astParent()->astParent()->previous(), "for ("))
condition = parent->astParent()->astParent()->previous();
else if (isBooleanFuncArg(tok))
else if (Token::Match(tok, "%comp%"))
condition = tok;
else
continue;
@ -1582,10 +1586,7 @@ void CheckCondition::alwaysTrueFalse()
if (hasSizeof)
continue;
auto it = std::find_if(tok->values().begin(), tok->values().end(), [](const ValueFlow::Value& v) {
return v.isIntValue();
});
alwaysTrueFalseError(tok, condition, &*it);
alwaysTrueFalseError(tok, condition, &tok->values().front());
}
}
}

2
lib/checkcondition.h
View File

@ -67,12 +67,12 @@ public:
checkCondition.checkPointerAdditionResultNotNull();
checkCondition.checkDuplicateConditionalAssign();
checkCondition.assignIf();
checkCondition.alwaysTrueFalse();
checkCondition.checkBadBitmaskCheck();
checkCondition.comparison();
checkCondition.checkModuloAlwaysTrueFalse();
checkCondition.checkAssignmentInCondition();
checkCondition.checkCompareValueOutOfTypeRange();
checkCondition.alwaysTrueFalse();
}
/** mismatching assignment / comparison */

100
lib/checkother.cpp
View File

@ -3640,6 +3640,21 @@ static bool isVariableExpression(const Token* tok)
return false;
}
static bool isVariableExprHidden(const Token* tok)
{
if (!tok)
return false;
if (!tok->astParent())
return false;
if (Token::simpleMatch(tok->astParent(), "*") && Token::simpleMatch(tok->astSibling(), "0"))
return true;
if (Token::simpleMatch(tok->astParent(), "&&") && Token::simpleMatch(tok->astSibling(), "false"))
return true;
if (Token::simpleMatch(tok->astParent(), "||") && Token::simpleMatch(tok->astSibling(), "true"))
return true;
return false;
}
void CheckOther::checkKnownArgument()
{
if (!mSettings->severity.isEnabled(Severity::style))
@ -3679,44 +3694,25 @@ void CheckOther::checkKnownArgument()
mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true))
continue;
// ensure that there is a integer variable in expression with unknown value
std::string varexpr;
bool isVariableExprHidden = false; // Is variable expression explicitly hidden
auto setVarExpr = [&varexpr, &isVariableExprHidden](const Token *child) {
const Token* vartok = findAstNode(tok, [](const Token* child) {
if (Token::Match(child, "%var%|.|[")) {
if (child->valueType() && child->valueType()->pointer == 0 && child->valueType()->isIntegral() && child->values().empty()) {
varexpr = child->expressionString();
return ChildrenToVisit::done;
}
return ChildrenToVisit::none;
return astIsIntegral(child, false) && !astIsPointer(child) && child->values().empty();
}
if (Token::simpleMatch(child->previous(), "sizeof ("))
return ChildrenToVisit::none;
// hide variable explicitly with 'x * 0' etc
if (!isVariableExprHidden) {
if (Token::simpleMatch(child, "*") && (Token::simpleMatch(child->astOperand1(), "0") || Token::simpleMatch(child->astOperand2(), "0")))
return ChildrenToVisit::none;
if (Token::simpleMatch(child, "&&") && (Token::simpleMatch(child->astOperand1(), "false") || Token::simpleMatch(child->astOperand2(), "false")))
return ChildrenToVisit::none;
if (Token::simpleMatch(child, "||") && (Token::simpleMatch(child->astOperand1(), "true") || Token::simpleMatch(child->astOperand2(), "true")))
return ChildrenToVisit::none;
}
return ChildrenToVisit::op1_and_op2;
};
visitAstNodes(tok, setVarExpr);
if (varexpr.empty()) {
isVariableExprHidden = true;
visitAstNodes(tok, setVarExpr);
}
if (varexpr.empty())
return false;
});
if (!vartok)
continue;
if (vartok->astSibling() &&
findAstNode(vartok->astSibling(), [](const Token* child) {
return Token::simpleMatch(child, "sizeof");
}))
continue;
// ensure that function name does not contain "assert"
std::string funcname = tok->astParent()->previous()->str();
std::string funcname = ftok->str();
strTolower(funcname);
if (funcname.find("assert") != std::string::npos)
continue;
knownArgumentError(tok, ftok, &tok->values().front(), varexpr, isVariableExprHidden);
knownArgumentError(tok, ftok, &tok->values().front(), vartok->expressionString(), isVariableExprHidden(vartok));
}
}
}
@ -3747,6 +3743,48 @@ void CheckOther::knownArgumentError(const Token *tok, const Token *ftok, const V
reportError(errorPath, Severity::style, id, errmsg, CWE570, Certainty::normal);
}
void CheckOther::checkKnownPointerToBool()
{
if (!mSettings->severity.isEnabled(Severity::style))
return;
const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase();
for (const Scope* functionScope : symbolDatabase->functionScopes) {
for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) {
if (!tok->hasKnownIntValue())
continue;
if (!astIsPointer(tok))
continue;
if (Token::Match(tok->astParent(), "?|!|&&|%oror%|%comp%"))
continue;
if (tok->astParent() && Token::Match(tok->astParent()->previous(), "if|while|switch|sizeof ("))
continue;
if (tok->isExpandedMacro())
continue;
if (findParent(tok, [](const Token* parent) {
return parent->isExpandedMacro();
}))
continue;
if (!isUsedAsBool(tok, mSettings))
continue;
const ValueFlow::Value& value = tok->values().front();
knownPointerToBoolError(tok, &value);
}
}
}
void CheckOther::knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value)
{
if (!tok) {
reportError(tok, Severity::style, "knownPointerToBool", "Pointer expression 'p' converted to bool is always true.");
return;
}
std::string cond = value->intvalue ? "true" : "false";
const std::string& expr = tok->expressionString();
std::string errmsg = "Pointer expression '" + expr + "' converted to bool is always " + cond + ".";
const ErrorPath errorPath = getErrorPath(tok, value, errmsg);
reportError(errorPath, Severity::style, "knownPointerToBool", errmsg, CWE570, Certainty::normal);
}
void CheckOther::checkComparePointers()
{
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();

5
lib/checkother.h
View File

@ -85,6 +85,7 @@ public:
checkOther.checkFuncArgNamesDifferent();
checkOther.checkShadowVariables();
checkOther.checkKnownArgument();
checkOther.checkKnownPointerToBool();
checkOther.checkComparePointers();
checkOther.checkIncompleteStatement();
checkOther.checkRedundantCopy();
@ -218,6 +219,8 @@ public:
void checkKnownArgument();
void checkKnownPointerToBool();
void checkComparePointers();
void checkModuloOfOne();
@ -279,6 +282,7 @@ private:
void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector<const Token*> & declarations, const std::vector<const Token*> & definitions);
void shadowError(const Token *var, const Token *shadowed, std::string type);
void knownArgumentError(const Token *tok, const Token *ftok, const ValueFlow::Value *value, const std::string &varexpr, bool isVariableExpressionHidden);
void knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value);
void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2);
void checkModuloOfOneError(const Token *tok);
@ -348,6 +352,7 @@ private:
c.shadowError(nullptr, nullptr, "function");
c.shadowError(nullptr, nullptr, "argument");
c.knownArgumentError(nullptr, nullptr, nullptr, "x", false);
c.knownPointerToBoolError(nullptr, nullptr);
c.comparePointersError(nullptr, nullptr, nullptr);
c.redundantAssignmentError(nullptr, nullptr, "var", false);
c.redundantInitializationError(nullptr, nullptr, "var", false);

2
lib/forwardanalyzer.cpp
View File

@ -488,7 +488,7 @@ struct ForwardTraversal {
if (allAnalysis.isIncremental())
return Break(Analyzer::Terminate::Bail);
} else if (allAnalysis.isModified()) {
std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, /*isModified*/ true);
std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
bool forkContinue = true;
for (ForwardTraversal& ft : ftv) {
if (condTok)

9
lib/token.cpp
View File

@ -2388,15 +2388,6 @@ bool Token::hasKnownIntValue() const
});
}
bool Token::hasKnownBoolValue() const
{
if (!mImpl->mValues)
return false;
return std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [](const ValueFlow::Value& value) {
return value.isIntValue() && (value.isKnown() || (value.intvalue == 0 && value.isImpossible()));
});
}
bool Token::hasKnownValue() const
{
return mImpl->mValues && std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), std::mem_fn(&ValueFlow::Value::isKnown));

1
lib/token.h
View File

@ -1216,7 +1216,6 @@ public:
}
bool hasKnownIntValue() const;
bool hasKnownBoolValue() const;
bool hasKnownValue() const;
bool hasKnownValue(ValueFlow::Value::ValueType t) const;
bool hasKnownSymbolicValue(const Token* tok) const;

3
lib/valueflow.cpp
View File

@ -6843,7 +6843,8 @@ static void valueFlowInferCondition(TokenList& tokenlist,
}
}
} else if (Token::Match(tok->astParent(), "?|&&|!|%oror%") ||
Token::Match(tok->astParent()->previous(), "if|while (")) {
Token::Match(tok->astParent()->previous(), "if|while (") ||
(astIsPointer(tok) && isUsedAsBool(tok, settings))) {
std::vector<ValueFlow::Value> result = infer(IntegralInferModel{}, "!=", tok->values(), 0);
if (result.size() != 1)
continue;

1
releasenotes.txt
View File

@ -2,6 +2,7 @@ Release Notes for Cppcheck 2.12
New checks:
- uselessOverride finds overriding functions that either duplicate code from or delegate back to the base class implementation
- knownPointerToBool finds pointer to bool conversions that are always true or false
Improved checking:
- truncLongCastAssignment and truncLongCastReturn check additional types, including float/double/long double

2
test/testastutils.cpp
View File

@ -484,7 +484,7 @@ private:
ASSERT(Result::False == isUsedAsBool("void f() { int i; if (i + 2) {} }", "i +"));
ASSERT(Result::True == isUsedAsBool("void f() { int i; bool b = i; }", "i ; }"));
ASSERT(Result::True == isUsedAsBool("void f(bool b); void f() { int i; f(i); }","i )"));
ASSERT(Result::True == isUsedAsBool("void f() { int *i; if (*i) {} }", "i )"));
ASSERT(Result::False == isUsedAsBool("void f() { int *i; if (*i) {} }", "i )"));
ASSERT(Result::True == isUsedAsBool("void f() { int *i; if (*i) {} }", "* i )"));
ASSERT(Result::True == isUsedAsBool("int g(); void h(bool); void f() { h(g()); }", "( ) )"));
ASSERT(Result::True == isUsedAsBool("int g(int); void h(bool); void f() { h(g(0)); }", "( 0 ) )"));

16
test/testcondition.cpp
View File

@ -1879,7 +1879,6 @@ private:
" return a % 5 > 5;\n"
"}");
ASSERT_EQUALS(
"[test.cpp:7]: (style) Return value 'a%5>5' is always false\n"
"[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n"
"[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n"
"[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n"
@ -1894,7 +1893,6 @@ private:
" b2 = x.a % 5 == 5;\n"
"}");
ASSERT_EQUALS(
"[test.cpp:3]: (style) Condition 'x[593]%5<=5' is always true\n"
"[test.cpp:2]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n"
"[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n"
"[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n",
@ -3223,7 +3221,9 @@ private:
" A(x++ == 1);\n"
" A(x++ == 2);\n"
"}");
TODO_ASSERT_EQUALS("function argument is always true? however is code really weird/suspicious?", "", errout.str());
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'x++==1' is always false\n"
"[test.cpp:4]: (style) Condition 'x++==2' is always false\n",
errout.str());
check("bool foo(int bar) {\n"
" bool ret = false;\n"
@ -4229,7 +4229,9 @@ private:
" if (w) {}\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'w' is always true\n", errout.str());
ASSERT_EQUALS("[test.cpp:4]: (style) Condition 'v<2' is always true\n"
"[test.cpp:5]: (style) Condition 'w' is always true\n",
errout.str());
check("void f(double d) {\n" // #10792
" if (d != 0) {\n"
@ -4521,13 +4523,9 @@ private:
" int* p = &i;\n"
" g(i == 7);\n"
" g(p == nullptr);\n"
" g(p);\n"
" g(&i);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5]: (style) Condition 'i==7' is always false\n"
"[test.cpp:6]: (style) Condition 'p==nullptr' is always false\n"
"[test.cpp:7]: (style) Condition 'p' is always true\n"
"[test.cpp:8]: (style) Condition '&i' is always true\n",
"[test.cpp:6]: (style) Condition 'p==nullptr' is always false\n",
errout.str());
}

69
test/testother.cpp
View File

@ -290,6 +290,8 @@ private:
TEST_CASE(checkOverlappingWrite);
TEST_CASE(constVariableArrayMember); // #10371
TEST_CASE(knownPointerToBool);
}
#define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
@ -11340,6 +11342,73 @@ private:
"};\n");
ASSERT_EQUALS("", errout.str());
}
void knownPointerToBool()
{
check("void g(bool);\n"
"void f() {\n"
" int i = 5;\n"
" int* p = &i;\n"
" g(p);\n"
" g(&i);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5]: (style) Pointer expression 'p' converted to bool is always true.\n"
"[test.cpp:6]: (style) Pointer expression '&i' converted to bool is always true.\n",
errout.str());
check("void f() {\n"
" const int* x = nullptr;\n"
" std::empty(x);\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("struct A { bool x; };\n"
"bool f(A* a) {\n"
" if (a) {\n"
" return a->x;\n"
" }\n"
" return false;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("struct A { int* x; };\n"
"bool f(A a) {\n"
" if (a.x) {\n"
" return a.x;\n"
" }\n"
" return false;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4]: (style) Pointer expression 'a.x' converted to bool is always true.\n", errout.str());
check("void f(bool* b) { if (b) *b = true; }");
ASSERT_EQUALS("", errout.str());
check("bool f() {\n"
" int* x = nullptr;\n"
" return bool(x);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str());
check("bool f() {\n"
" int* x = nullptr;\n"
" return bool{x};\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str());
check("struct A { A(bool); };\n"
"A f() {\n"
" int* x = nullptr;\n"
" return A(x);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str());
check("struct A { A(bool); };\n"
"A f() {\n"
" int* x = nullptr;\n"
" return A{x};\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4]: (style) Pointer expression 'x' converted to bool is always false.\n", errout.str());
}
};
REGISTER_TEST(TestOther)