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

Fix issue 9541: false negative: knownConditionTrueFalse (#2473) 

* Fix issue 9541: false negative: knownConditionTrueFalse

* Add another test case

* Add another test

* Fix FPs

* Format

* Fix compile error

* Remove double conditions

* Fix compile error
This commit is contained in:
Paul Fultz II 2020-01-05 09:25:33 -06:00 committed by Daniel Marjamäki
parent 5f73e1cb32
commit 90f82d0374
5 changed files with 162 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
lib/programmemory.cpp
View File

@ -214,13 +214,36 @@ static void fillProgramMemoryFromAssignments(ProgramMemory& pm, const Token* tok
}
}
static void removeModifiedVars(ProgramMemory& pm, const Token* tok, const Token* origin)
{
for (auto i = pm.values.begin(), last = pm.values.end(); i != last;) {
if (isVariableChanged(origin, tok, i->first, false, nullptr, true)) {
i = pm.values.erase(i);
} else {
++i;
}
}
}
static ProgramMemory getInitialProgramState(const Token* tok,
const Token* origin,
const std::unordered_map<nonneg int, ValueFlow::Value>& vars = std::unordered_map<nonneg int, ValueFlow::Value>{})
{
ProgramMemory pm;
if (origin) {
fillProgramMemoryFromConditions(pm, origin, nullptr);
const ProgramMemory state = pm;
fillProgramMemoryFromAssignments(pm, tok, state, vars);
removeModifiedVars(pm, tok, origin);
}
return pm;
}
ProgramMemory getProgramMemory(const Token *tok, nonneg int varid, const ValueFlow::Value &value)
{
ProgramMemory programMemory;
if (value.tokvalue)
fillProgramMemoryFromConditions(programMemory, value.tokvalue, nullptr);
if (value.condition)
fillProgramMemoryFromConditions(programMemory, value.condition, nullptr);
programMemory.replace(getInitialProgramState(tok, value.tokvalue));
programMemory.replace(getInitialProgramState(tok, value.condition));
programMemory.setValue(varid, value);
if (value.varId)
programMemory.setIntValue(value.varId, value.varvalue);

79
lib/valueflow.cpp
View File

@ -2178,6 +2178,44 @@ static void valueFlowForwardExpression(Token* startToken,
}
}
static bool bifurcate(const Token* tok, const std::set<nonneg int>& varids, const Settings* settings, int depth = 20)
{
if (depth < 0)
return false;
if (!tok)
return true;
if (tok->hasKnownIntValue())
return true;
if (Token::Match(tok, "%cop%"))
return bifurcate(tok->astOperand1(), varids, settings) && bifurcate(tok->astOperand2(), varids, settings, depth);
if (Token::Match(tok, "%var%")) {
if (varids.count(tok->varId()) > 0)
return true;
const Variable* var = tok->variable();
if (!var)
return false;
const Token* start = var->declEndToken();
if (!start)
return false;
if (Token::Match(start, "; %varid% =", var->declarationId()))
start = start->tokAt(2);
if (var->isConst() ||
!isVariableChanged(start->next(), tok, var->declarationId(), var->isGlobal(), settings, true))
return var->isArgument() || bifurcate(start->astOperand2(), varids, settings, depth - 1);
return false;
}
return false;
}
static void addCondition(std::list<ValueFlow::Value>& values, const Token* condTok, bool assume)
{
for (ValueFlow::Value& v : values)
if (assume)
v.errorPath.emplace_back(condTok, "Assuming condition is true.");
else
v.errorPath.emplace_back(condTok, "Assuming condition is false.");
}
static bool valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
@ -2393,9 +2431,21 @@ static bool valueFlowForwardVariable(Token* const startToken,
falsevalues.push_back(v);
}
bool unknown = false;
if (truevalues.empty() && falsevalues.empty() && condTok &&
std::all_of(
condTok->values().begin(), condTok->values().end(), std::mem_fn(&ValueFlow::Value::isNonValue)) &&
bifurcate(condTok, {varid}, settings)) {
truevalues = values;
addCondition(truevalues, condTok, true);
falsevalues = values;
addCondition(falsevalues, condTok, false);
unknown = true;
}
if (!truevalues.empty() || !falsevalues.empty()) {
Token* tok3 = tok2;
// '{'
const Token * const startToken1 = tok2->linkAt(1)->next();
const Token* const startToken1 = tok3->linkAt(1)->next();
bool vfresult = valueFlowForwardVariable(startToken1->next(),
startToken1->link(),
@ -2408,22 +2458,24 @@ static bool valueFlowForwardVariable(Token* const startToken,
errorLogger,
settings);
if (!condAlwaysFalse && isVariableChanged(startToken1, startToken1->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) {
if (!unknown && !condAlwaysFalse &&
isVariableChanged(
startToken1, startToken1->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) {
removeValues(values, truevalues);
lowerToPossible(values);
}
// goto '}'
tok2 = startToken1->link();
tok3 = startToken1->link();
if (isEscapeScope(startToken1, tokenlist, true) || !vfresult) {
if (!unknown && (isEscapeScope(startToken1, tokenlist, true) || !vfresult)) {
if (condAlwaysTrue)
return false;
removeValues(values, truevalues);
}
if (Token::simpleMatch(tok2, "} else {")) {
const Token * const startTokenElse = tok2->tokAt(2);
if (Token::simpleMatch(tok3, "} else {")) {
const Token* const startTokenElse = tok3->tokAt(2);
vfresult = valueFlowForwardVariable(startTokenElse->next(),
startTokenElse->link(),
@ -2436,15 +2488,17 @@ static bool valueFlowForwardVariable(Token* const startToken,
errorLogger,
settings);
if (!condAlwaysTrue && isVariableChanged(startTokenElse, startTokenElse->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) {
if (!unknown && !condAlwaysTrue &&
isVariableChanged(
startTokenElse, startTokenElse->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) {
removeValues(values, falsevalues);
lowerToPossible(values);
}
// goto '}'
tok2 = startTokenElse->link();
tok3 = startTokenElse->link();
if (isEscapeScope(startTokenElse, tokenlist, true) || !vfresult) {
if (!unknown && (isEscapeScope(startTokenElse, tokenlist, true) || !vfresult)) {
if (condAlwaysFalse)
return false;
removeValues(values, falsevalues);
@ -2452,7 +2506,10 @@ static bool valueFlowForwardVariable(Token* const startToken,
}
if (values.empty())
return false;
continue;
if (!unknown) {
tok2 = tok3;
continue;
}
}
Token * const start = tok2->linkAt(1)->next();
@ -6261,9 +6318,9 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
valueFlowTerminatingCondition(tokenlist, symboldatabase, errorLogger, settings);
valueFlowBeforeCondition(tokenlist, symboldatabase, errorLogger, settings);
valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings);
valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);
valueFlowAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
valueFlowInferCondition(tokenlist, settings);
valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);
valueFlowSwitchVariable(tokenlist, symboldatabase, errorLogger, settings);
valueFlowForLoop(tokenlist, symboldatabase, errorLogger, settings);
valueFlowSubFunction(tokenlist, errorLogger, settings);

11
test/testcondition.cpp
View File

@ -3474,6 +3474,17 @@ private:
" }\n"
"}\n");
ASSERT_EQUALS("", errout.str());
// #9541
check("int f(int pos, int a) {\n"
" if (pos <= 0)\n"
" pos = 0;\n"
" else if (pos < a)\n"
" if(pos > 0)\n"
" --pos;\n"
" return pos;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:5]: (style) Condition 'pos>0' is always true\n", errout.str());
}
void alwaysTrueContainer() {

55
test/testnullpointer.cpp
View File

@ -88,6 +88,8 @@ private:
TEST_CASE(nullpointer46); // #9441
TEST_CASE(nullpointer47); // #6850
TEST_CASE(nullpointer48); // #9196
TEST_CASE(nullpointer49); // #7804
TEST_CASE(nullpointer50); // #6462
TEST_CASE(nullpointer_addressOf); // address of
TEST_CASE(nullpointerSwitch); // #2626
TEST_CASE(nullpointer_cast); // #4692
@ -1448,6 +1450,19 @@ private:
" g(0);\n"
"}\n", true);
ASSERT_EQUALS("", errout.str());
check("bool f(int*);\n"
"void g(int* x) {\n"
" bool b = f(x);\n"
" if (b) {\n"
" *x = 1;\n"
" }\n"
"}\n"
"void h() {\n"
" g(0);\n"
"}\n",
true);
ASSERT_EQUALS("", errout.str());
}
void nullpointer36() {
@ -1643,6 +1658,42 @@ private:
ASSERT_EQUALS("", errout.str());
}
void nullpointer49()
{
check("void f(int *p, int n) {\n"
" int *q = 0;\n"
" if(n > 10) q = p;\n"
" *p +=2;\n"
" if(n < 120) *q+=12;\n"
"}\n");
TODO_ASSERT_EQUALS("[test.cpp:5]: (warning) Possible null pointer dereference: q\n", "", errout.str());
check("void f(int *p, int n) {\n"
" int *q = 0;\n"
" if(n > 10) q = p;\n"
" *p +=2;\n"
" if(n > 10) *q+=12;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void nullpointer50()
{
check("void f(int *p, int a) {\n"
" if(!p) {\n"
" if(a > 0) {\n"
" if(a > 10){}\n"
" else {\n"
" *p = 0;\n"
" }\n"
" }\n"
" }\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:2] -> [test.cpp:6]: (warning) Either the condition '!p' is redundant or there is possible null pointer dereference: p.\n",
errout.str());
}
void nullpointer_addressOf() { // address of
check("void f() {\n"
" struct X *x = 0;\n"
@ -2834,7 +2885,9 @@ private:
" if (a != 0)\n"
" *p = 0;\n"
"}", true);
TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str());
ASSERT_EQUALS(
"[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n",
errout.str());
check("void f(int *p = 0) {\n"
" p = a;\n"

4
test/testvalueflow.cpp
View File

@ -1676,7 +1676,7 @@ private:
" ++x;\n"
" return 2 + x;\n"
"}";
ASSERT_EQUALS(false, testValueOfX(code, 4U, 123));
ASSERT_EQUALS(true, testValueOfX(code, 4U, 123));
code = "void f() {\n"
" int x = 1;\n"
@ -1839,7 +1839,7 @@ private:
" else if (x && i == 1) {}\n"
" }\n"
"}\n";
ASSERT_EQUALS(false, testValueOfX(code, 6U, 0));
ASSERT_EQUALS(true, testValueOfX(code, 6U, 0));
// multivariables
code = "void f(int a) {\n"