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

Fix issue 8373: false negative: invalid iterator (#2761)

This commit is contained in:
Paul Fultz II 2020-08-31 01:46:56 -05:00 committed by GitHub
parent dc6f0740c1
commit 1c5f496350
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 142 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
lib/checkcondition.cpp
View File

@ -54,7 +54,16 @@ bool CheckCondition::diag(const Token* tok, bool insert)
{
if (!tok)
return false;
if (mCondDiags.find(tok) == mCondDiags.end()) {
const Token* parent = tok->astParent();
bool hasParent = false;
while (Token::Match(parent, "&&|%oror%")) {
if (mCondDiags.count(parent) != 0) {
hasParent = true;
break;
}
parent = parent->astParent();
}
if (mCondDiags.count(tok) == 0 && !hasParent) {
if (insert)
mCondDiags.insert(tok);
return false;
@ -1239,6 +1248,8 @@ void CheckCondition::checkIncorrectLogicOperator()
void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive, ErrorPath errors)
{
if (diag(tok))
return;
errors.emplace_back(tok, "");
if (always)
reportError(errors, Severity::warning, "incorrectLogicOperator",
@ -1254,6 +1265,8 @@ void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::st
void CheckCondition::redundantConditionError(const Token *tok, const std::string &text, bool inconclusive)
{
if (diag(tok))
return;
reportError(tok, Severity::style, "redundantCondition", "Redundant condition: " + text, CWE398, inconclusive);
}

2
lib/checkio.cpp
View File

@ -1223,7 +1223,7 @@ void CheckIO::checkFormatString(const Token * const tok,
case 'l': { // Can be 'll' (long long int or unsigned long long int) or 'l' (long int or unsigned long int)
// If the next character is the same (which makes 'hh' or 'll') then expect another alphabetical character
if (i != formatString.end() && (i + 1) != formatString.end() && *(i + 1) == *i) {
if (!isalpha(*(i + 2))) {
if ((i + 2) != formatString.end() && !isalpha(*(i + 2))) {
std::string modifier;
modifier += *i;
modifier += *(i + 1);

89
lib/valueflow.cpp
View File

@ -2835,7 +2835,7 @@ template <class F>
void transformIntValues(std::list<ValueFlow::Value>& values, F f)
{
std::transform(values.begin(), values.end(), values.begin(), [&](ValueFlow::Value x) {
if (x.isIntValue())
if (x.isIntValue() || x.isIteratorValue())
x.intvalue = f(x.intvalue);
return x;
});
@ -4286,6 +4286,26 @@ struct ValueFlowConditionHandler {
continue;
}
std::list<ValueFlow::Value> thenValues;
std::list<ValueFlow::Value> elseValues;
if (!Token::Match(tok, "!=|=|(|.") && tok != cond.vartok) {
thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end());
if (isConditionKnown(tok, false))
insertImpossible(elseValues, cond.false_values);
}
if (!Token::Match(tok, "==|!")) {
elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end());
if (isConditionKnown(tok, true)) {
insertImpossible(thenValues, cond.true_values);
if (Token::Match(tok, "(|.|%var%") && astIsBool(tok))
insertNegateKnown(thenValues, cond.true_values);
}
}
if (cond.inverted)
std::swap(thenValues, elseValues);
if (Token::Match(tok->astParent(), "%oror%|&&")) {
Token *parent = tok->astParent();
if (astIsRHS(tok) && parent->astParent() && parent->str() == parent->astParent()->str())
@ -4295,11 +4315,14 @@ struct ValueFlowConditionHandler {
}
if (parent) {
const std::string &op(parent->str());
std::list<ValueFlow::Value> values = cond.true_values;
std::list<ValueFlow::Value> values;
if (op == "&&")
values = thenValues;
else if (op == "||")
values = elseValues;
if (Token::Match(tok, "==|!="))
changePossibleToKnown(values);
if ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) ||
(op == "||" && Token::Match(tok, "%name%|!="))) {
if (!values.empty()) {
bool assign = false;
visitAstNodes(parent->astOperand2(), [&](Token* tok2) {
if (isSameExpression(tokenlist->isCPP(), false, cond.vartok, tok2, settings->library, true, false))
@ -4331,26 +4354,6 @@ struct ValueFlowConditionHandler {
continue;
}
std::list<ValueFlow::Value> thenValues;
std::list<ValueFlow::Value> elseValues;
if (!Token::Match(tok, "!=|=|(|.") && tok != cond.vartok) {
thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end());
if (isConditionKnown(tok, false))
insertImpossible(elseValues, cond.false_values);
}
if (!Token::Match(tok, "==|!")) {
elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end());
if (isConditionKnown(tok, true)) {
insertImpossible(thenValues, cond.true_values);
if (Token::Match(tok, "(|.|%var%") && astIsBool(tok))
insertNegateKnown(thenValues, cond.true_values);
}
}
if (cond.inverted)
std::swap(thenValues, elseValues);
// start token of conditional code
Token* startTokens[] = {nullptr, nullptr};
@ -5943,10 +5946,12 @@ static void valueFlowIterators(TokenList *tokenlist, const Settings *settings)
}
}
static std::list<ValueFlow::Value> getIteratorValues(std::list<ValueFlow::Value> values)
static std::list<ValueFlow::Value> getIteratorValues(std::list<ValueFlow::Value> values, ValueFlow::Value::ValueKind* kind = nullptr)
{
values.remove_if([&](const ValueFlow::Value& v) {
return !v.isIteratorEndValue() && !v.isIteratorStartValue();
if (kind && v.valueKind != *kind)
return true;
return !v.isIteratorValue();
});
return values;
}
@ -5976,7 +5981,8 @@ static void valueFlowIteratorAfterCondition(TokenList *tokenlist,
if (!tok->astOperand1() || !tok->astOperand2())
return cond;
std::list<ValueFlow::Value> values = getIteratorValues(tok->astOperand1()->values());
ValueFlow::Value::ValueKind kind = ValueFlow::Value::ValueKind::Known;
std::list<ValueFlow::Value> values = getIteratorValues(tok->astOperand1()->values(), &kind);
if (!values.empty()) {
cond.vartok = tok->astOperand2();
} else {
@ -5997,6 +6003,34 @@ static void valueFlowIteratorAfterCondition(TokenList *tokenlist,
handler.afterCondition(tokenlist, symboldatabase, errorLogger, settings);
}
static void valueFlowIteratorInfer(TokenList *tokenlist, const Settings *settings)
{
for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
if (!tok->scope())
continue;
if (!tok->scope()->isExecutable())
continue;
std::list<ValueFlow::Value> values = getIteratorValues(tok->values());
values.remove_if([&](const ValueFlow::Value& v) {
if (!v.isImpossible())
return true;
if (v.isIteratorEndValue() && v.intvalue <= 0)
return true;
if (v.isIteratorStartValue() && v.intvalue >= 0)
return true;
return false;
});
for(ValueFlow::Value& v:values) {
v.setPossible();
if (v.isIteratorStartValue())
v.intvalue++;
if (v.isIteratorEndValue())
v.intvalue--;
setTokenValue(tok, v, settings);
}
}
}
static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings)
{
// declaration
@ -6600,6 +6634,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
valueFlowSmartPointer(tokenlist, errorLogger, settings);
valueFlowIterators(tokenlist, settings);
valueFlowIteratorAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
valueFlowIteratorInfer(tokenlist, settings);
valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings);
valueFlowContainerAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
}

33
test/testcondition.cpp
View File

@ -840,16 +840,14 @@ private:
" a++;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always true\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str());
check("void f(int x) {\n"
" if (1 != x || 3 != x)\n"
" a++;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition '3!=x' is always true\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str());
check("void f(int x) {\n"
" if (x<0 && !x) {}\n"
@ -859,14 +857,12 @@ private:
check("void f(int x) {\n"
" if (x==0 && x) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n", errout.str());
check("void f(int x) {\n" // ast..
" if (y == 1 && x == 1 && x == 7) { }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==7' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n", errout.str());
check("void f(int x, int y) {\n"
" if (x != 1 || y != 1)\n"
@ -930,8 +926,7 @@ private:
" a++;\n"
"}\n"
);
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=6' is always true\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n", errout.str());
check("void f(unsigned int a, unsigned int b, unsigned int c) {\n"
" if((a != b) || (c != b) || (c != a))\n"
@ -961,8 +956,7 @@ private:
" if (x == 1 && x == 3)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n", errout.str());
check("void f(int x) {\n"
" if (x == 1.0 && x == 3.0)\n"
@ -1085,8 +1079,7 @@ private:
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=4' is always true\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
check("void f(int x) {\n"
" if ((x!=4) && (x==3))\n"
@ -1104,15 +1097,13 @@ private:
" if ((x!=4) || (x==3))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
check("void f(int x) {\n"
" if ((x==3) && (x!=3))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n", errout.str());
check("void f(int x) {\n"
" if ((x==6) || (x!=6))\n"
@ -1204,8 +1195,7 @@ private:
check("void f(char x) {\n"
" if (x == '1' && x == '2') {}\n"
"}", "test.cpp", true);
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x=='2'' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n", errout.str());
check("int f(char c) {\n"
" return (c >= 'a' && c <= 'z');\n"
@ -1279,8 +1269,7 @@ private:
" if ((t == A) && (t == B))\n"
" {}\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n"
"[test.cpp:3] -> [test.cpp:3]: (style) Condition 't==B' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n", errout.str());
}
void incorrectLogicOperator11() {

54
test/teststl.cpp
View File

@ -3634,12 +3634,66 @@ private:
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n"
"[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i.\n", errout.str());
check("void f(std::vector<int> & v) {\n"
" std::vector<int>::iterator i= v.begin();\n"
" if(i == v.end() && *i == *(i+1)) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i.\n"
"[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n", errout.str());
check("void f(std::vector<int> & v) {\n"
" std::vector<int>::iterator i= v.begin();\n"
" if(i != v.end() && *i == *(i+1)) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3]: (warning) Either the condition 'i!=v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n", errout.str());
check("void f(std::vector<int> & v) {\n"
" std::vector<int>::iterator i= v.begin();\n"
" if(i != v.end()) {\n"
" if (*(i+1) == *i) {}\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'i!=v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n", errout.str());
check("void f(std::vector<int> & v) {\n"
" std::vector<int>::iterator i= v.begin();\n"
" if(i == v.end()) { return; }\n"
" if (*(i+1) == *i) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition 'i==v.end()' is redundant or there is possible dereference of an invalid iterator: i+1.\n", errout.str());
check("void f(std::vector<int> & v) {\n"
" std::vector<int>::iterator i= v.begin();\n"
" if(i != v.end() && (i+1) != v.end() && *(i+1) == *i) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("void f(std::string s) {\n"
" for (std::string::const_iterator i = s.begin(); i != s.end(); ++i) {\n"
" if (i != s.end() && (i + 1) != s.end() && *(i + 1) == *i) {\n"
" if (!isalpha(*(i + 2))) {\n"
" std::string modifier;\n"
" modifier += *i;\n"
" modifier += *(i + 1);\n"
" }\n"
" }\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition '(i+1)!=s.end()' is redundant or there is possible dereference of an invalid iterator: i+2.\n", errout.str());
check("void f(std::string s) {\n"
" for (std::string::const_iterator i = s.begin(); i != s.end(); ++i) {\n"
" if (i != s.end() && (i + 1) != s.end() && *(i + 1) == *i) {\n"
" if ((i + 2) != s.end() && !isalpha(*(i + 2))) {\n"
" std::string modifier;\n"
" modifier += *i;\n"
" modifier += *(i + 1);\n"
" }\n"
" }\n"
" }\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void dereferenceInvalidIterator2() {