Skip to content

Commit 1c5f496

Browse files
pfultz2pfultz2
authored
Fix issue 8373: false negative: invalid iterator (cppcheck-opensource#2761)
1 parent dc6f074 commit 1c5f496

5 files changed

Lines changed: 142 additions & 51 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,16 @@ bool CheckCondition::diag(const Token* tok, bool insert)
5454
{
5555
if (!tok)
5656
return false;
57-
if (mCondDiags.find(tok) == mCondDiags.end()) {
57+
const Token* parent = tok->astParent();
58+
bool hasParent = false;
59+
while (Token::Match(parent, "&&|%oror%")) {
60+
if (mCondDiags.count(parent) != 0) {
61+
hasParent = true;
62+
break;
63+
}
64+
parent = parent->astParent();
65+
}
66+
if (mCondDiags.count(tok) == 0 && !hasParent) {
5867
if (insert)
5968
mCondDiags.insert(tok);
6069
return false;
@@ -1239,6 +1248,8 @@ void CheckCondition::checkIncorrectLogicOperator()
12391248

12401249
void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive, ErrorPath errors)
12411250
{
1251+
if (diag(tok))
1252+
return;
12421253
errors.emplace_back(tok, "");
12431254
if (always)
12441255
reportError(errors, Severity::warning, "incorrectLogicOperator",
@@ -1254,6 +1265,8 @@ void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::st
12541265

12551266
void CheckCondition::redundantConditionError(const Token *tok, const std::string &text, bool inconclusive)
12561267
{
1268+
if (diag(tok))
1269+
return;
12571270
reportError(tok, Severity::style, "redundantCondition", "Redundant condition: " + text, CWE398, inconclusive);
12581271
}
12591272

‎lib/checkio.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1223,7 +1223,7 @@ void CheckIO::checkFormatString(const Token * const tok,
12231223
case 'l': { // Can be 'll' (long long int or unsigned long long int) or 'l' (long int or unsigned long int)
12241224
// If the next character is the same (which makes 'hh' or 'll') then expect another alphabetical character
12251225
if (i != formatString.end() && (i + 1) != formatString.end() && *(i + 1) == *i) {
1226-
if (!isalpha(*(i + 2))) {
1226+
if ((i + 2) != formatString.end() && !isalpha(*(i + 2))) {
12271227
std::string modifier;
12281228
modifier += *i;
12291229
modifier += *(i + 1);

‎lib/valueflow.cpp‎

Lines changed: 62 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2835,7 +2835,7 @@ template <class F>
28352835
void transformIntValues(std::list<ValueFlow::Value>& values, F f)
28362836
{
28372837
std::transform(values.begin(), values.end(), values.begin(), [&](ValueFlow::Value x) {
2838-
if (x.isIntValue())
2838+
if (x.isIntValue() || x.isIteratorValue())
28392839
x.intvalue = f(x.intvalue);
28402840
return x;
28412841
});
@@ -4286,6 +4286,26 @@ struct ValueFlowConditionHandler {
42864286
continue;
42874287
}
42884288

4289+
std::list<ValueFlow::Value> thenValues;
4290+
std::list<ValueFlow::Value> elseValues;
4291+
4292+
if (!Token::Match(tok, "!=|=|(|.") && tok != cond.vartok) {
4293+
thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end());
4294+
if (isConditionKnown(tok, false))
4295+
insertImpossible(elseValues, cond.false_values);
4296+
}
4297+
if (!Token::Match(tok, "==|!")) {
4298+
elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end());
4299+
if (isConditionKnown(tok, true)) {
4300+
insertImpossible(thenValues, cond.true_values);
4301+
if (Token::Match(tok, "(|.|%var%") && astIsBool(tok))
4302+
insertNegateKnown(thenValues, cond.true_values);
4303+
}
4304+
}
4305+
4306+
if (cond.inverted)
4307+
std::swap(thenValues, elseValues);
4308+
42894309
if (Token::Match(tok->astParent(), "%oror%|&&")) {
42904310
Token *parent = tok->astParent();
42914311
if (astIsRHS(tok) && parent->astParent() && parent->str() == parent->astParent()->str())
@@ -4295,11 +4315,14 @@ struct ValueFlowConditionHandler {
42954315
}
42964316
if (parent) {
42974317
const std::string &op(parent->str());
4298-
std::list<ValueFlow::Value> values = cond.true_values;
4318+
std::list<ValueFlow::Value> values;
4319+
if (op == "&&")
4320+
values = thenValues;
4321+
else if (op == "||")
4322+
values = elseValues;
42994323
if (Token::Match(tok, "==|!="))
43004324
changePossibleToKnown(values);
4301-
if ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) ||
4302-
(op == "||" && Token::Match(tok, "%name%|!="))) {
4325+
if (!values.empty()) {
43034326
bool assign = false;
43044327
visitAstNodes(parent->astOperand2(), [&](Token* tok2) {
43054328
if (isSameExpression(tokenlist->isCPP(), false, cond.vartok, tok2, settings->library, true, false))
@@ -4331,26 +4354,6 @@ struct ValueFlowConditionHandler {
43314354
continue;
43324355
}
43334356

4334-
std::list<ValueFlow::Value> thenValues;
4335-
std::list<ValueFlow::Value> elseValues;
4336-
4337-
if (!Token::Match(tok, "!=|=|(|.") && tok != cond.vartok) {
4338-
thenValues.insert(thenValues.end(), cond.true_values.begin(), cond.true_values.end());
4339-
if (isConditionKnown(tok, false))
4340-
insertImpossible(elseValues, cond.false_values);
4341-
}
4342-
if (!Token::Match(tok, "==|!")) {
4343-
elseValues.insert(elseValues.end(), cond.false_values.begin(), cond.false_values.end());
4344-
if (isConditionKnown(tok, true)) {
4345-
insertImpossible(thenValues, cond.true_values);
4346-
if (Token::Match(tok, "(|.|%var%") && astIsBool(tok))
4347-
insertNegateKnown(thenValues, cond.true_values);
4348-
}
4349-
}
4350-
4351-
if (cond.inverted)
4352-
std::swap(thenValues, elseValues);
4353-
43544357
// start token of conditional code
43554358
Token* startTokens[] = {nullptr, nullptr};
43564359

@@ -5943,10 +5946,12 @@ static void valueFlowIterators(TokenList *tokenlist, const Settings *settings)
59435946
}
59445947
}
59455948

5946-
static std::list<ValueFlow::Value> getIteratorValues(std::list<ValueFlow::Value> values)
5949+
static std::list<ValueFlow::Value> getIteratorValues(std::list<ValueFlow::Value> values, ValueFlow::Value::ValueKind* kind = nullptr)
59475950
{
59485951
values.remove_if([&](const ValueFlow::Value& v) {
5949-
return !v.isIteratorEndValue() && !v.isIteratorStartValue();
5952+
if (kind && v.valueKind != *kind)
5953+
return true;
5954+
return !v.isIteratorValue();
59505955
});
59515956
return values;
59525957
}
@@ -5976,7 +5981,8 @@ static void valueFlowIteratorAfterCondition(TokenList *tokenlist,
59765981
if (!tok->astOperand1() || !tok->astOperand2())
59775982
return cond;
59785983

5979-
std::list<ValueFlow::Value> values = getIteratorValues(tok->astOperand1()->values());
5984+
ValueFlow::Value::ValueKind kind = ValueFlow::Value::ValueKind::Known;
5985+
std::list<ValueFlow::Value> values = getIteratorValues(tok->astOperand1()->values(), &kind);
59805986
if (!values.empty()) {
59815987
cond.vartok = tok->astOperand2();
59825988
} else {
@@ -5997,6 +6003,34 @@ static void valueFlowIteratorAfterCondition(TokenList *tokenlist,
59976003
handler.afterCondition(tokenlist, symboldatabase, errorLogger, settings);
59986004
}
59996005

6006+
static void valueFlowIteratorInfer(TokenList *tokenlist, const Settings *settings)
6007+
{
6008+
for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
6009+
if (!tok->scope())
6010+
continue;
6011+
if (!tok->scope()->isExecutable())
6012+
continue;
6013+
std::list<ValueFlow::Value> values = getIteratorValues(tok->values());
6014+
values.remove_if([&](const ValueFlow::Value& v) {
6015+
if (!v.isImpossible())
6016+
return true;
6017+
if (v.isIteratorEndValue() && v.intvalue <= 0)
6018+
return true;
6019+
if (v.isIteratorStartValue() && v.intvalue >= 0)
6020+
return true;
6021+
return false;
6022+
});
6023+
for(ValueFlow::Value& v:values) {
6024+
v.setPossible();
6025+
if (v.isIteratorStartValue())
6026+
v.intvalue++;
6027+
if (v.isIteratorEndValue())
6028+
v.intvalue--;
6029+
setTokenValue(tok, v, settings);
6030+
}
6031+
}
6032+
}
6033+
60006034
static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger * /*errorLogger*/, const Settings *settings)
60016035
{
60026036
// declaration
@@ -6600,6 +6634,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
66006634
valueFlowSmartPointer(tokenlist, errorLogger, settings);
66016635
valueFlowIterators(tokenlist, settings);
66026636
valueFlowIteratorAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
6637+
valueFlowIteratorInfer(tokenlist, settings);
66036638
valueFlowContainerSize(tokenlist, symboldatabase, errorLogger, settings);
66046639
valueFlowContainerAfterCondition(tokenlist, symboldatabase, errorLogger, settings);
66056640
}

‎test/testcondition.cpp‎

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -840,16 +840,14 @@ class TestCondition : public TestFixture {
840840
" a++;\n"
841841
"}\n"
842842
);
843-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n"
844-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always true\n", errout.str());
843+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str());
845844

846845
check("void f(int x) {\n"
847846
" if (1 != x || 3 != x)\n"
848847
" a++;\n"
849848
"}\n"
850849
);
851-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n"
852-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition '3!=x' is always true\n", errout.str());
850+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str());
853851

854852
check("void f(int x) {\n"
855853
" if (x<0 && !x) {}\n"
@@ -859,14 +857,12 @@ class TestCondition : public TestFixture {
859857
check("void f(int x) {\n"
860858
" if (x==0 && x) {}\n"
861859
"}\n");
862-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n"
863-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x' is always false\n", errout.str());
860+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n", errout.str());
864861

865862
check("void f(int x) {\n" // ast..
866863
" if (y == 1 && x == 1 && x == 7) { }\n"
867864
"}\n");
868-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n"
869-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==7' is always false\n", errout.str());
865+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n", errout.str());
870866

871867
check("void f(int x, int y) {\n"
872868
" if (x != 1 || y != 1)\n"
@@ -930,8 +926,7 @@ class TestCondition : public TestFixture {
930926
" a++;\n"
931927
"}\n"
932928
);
933-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n"
934-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=6' is always true\n", errout.str());
929+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n", errout.str());
935930

936931
check("void f(unsigned int a, unsigned int b, unsigned int c) {\n"
937932
" if((a != b) || (c != b) || (c != a))\n"
@@ -961,8 +956,7 @@ class TestCondition : public TestFixture {
961956
" if (x == 1 && x == 3)\n"
962957
" a++;\n"
963958
"}");
964-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n"
965-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str());
959+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n", errout.str());
966960

967961
check("void f(int x) {\n"
968962
" if (x == 1.0 && x == 3.0)\n"
@@ -1085,8 +1079,7 @@ class TestCondition : public TestFixture {
10851079
" a++;\n"
10861080
"}");
10871081

1088-
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n"
1089-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=4' is always true\n", errout.str());
1082+
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
10901083

10911084
check("void f(int x) {\n"
10921085
" if ((x!=4) && (x==3))\n"
@@ -1104,15 +1097,13 @@ class TestCondition : public TestFixture {
11041097
" if ((x!=4) || (x==3))\n"
11051098
" a++;\n"
11061099
"}");
1107-
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n"
1108-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str());
1100+
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
11091101

11101102
check("void f(int x) {\n"
11111103
" if ((x==3) && (x!=3))\n"
11121104
" a++;\n"
11131105
"}");
1114-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n"
1115-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always false\n", errout.str());
1106+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n", errout.str());
11161107

11171108
check("void f(int x) {\n"
11181109
" if ((x==6) || (x!=6))\n"
@@ -1204,8 +1195,7 @@ class TestCondition : public TestFixture {
12041195
check("void f(char x) {\n"
12051196
" if (x == '1' && x == '2') {}\n"
12061197
"}", "test.cpp", true);
1207-
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n"
1208-
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x=='2'' is always false\n", errout.str());
1198+
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n", errout.str());
12091199

12101200
check("int f(char c) {\n"
12111201
" return (c >= 'a' && c <= 'z');\n"
@@ -1279,8 +1269,7 @@ class TestCondition : public TestFixture {
12791269
" if ((t == A) && (t == B))\n"
12801270
" {}\n"
12811271
"}");
1282-
ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n"
1283-
"[test.cpp:3] -> [test.cpp:3]: (style) Condition 't==B' is always false\n", errout.str());
1272+
ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n", errout.str());
12841273
}
12851274

12861275
void incorrectLogicOperator11() {

‎test/teststl.cpp‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3634,12 +3634,66 @@ class TestStl : public TestFixture {
36343634
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"
36353635
"[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());
36363636

3637+
36373638
check("void f(std::vector<int> & v) {\n"
36383639
" std::vector<int>::iterator i= v.begin();\n"
36393640
" if(i == v.end() && *i == *(i+1)) {}\n"
36403641
"}\n");
36413642
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"
36423643
"[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());
3644+
3645+
check("void f(std::vector<int> & v) {\n"
3646+
" std::vector<int>::iterator i= v.begin();\n"
3647+
" if(i != v.end() && *i == *(i+1)) {}\n"
3648+
"}\n");
3649+
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());
3650+
3651+
check("void f(std::vector<int> & v) {\n"
3652+
" std::vector<int>::iterator i= v.begin();\n"
3653+
" if(i != v.end()) {\n"
3654+
" if (*(i+1) == *i) {}\n"
3655+
" }\n"
3656+
"}\n");
3657+
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());
3658+
3659+
check("void f(std::vector<int> & v) {\n"
3660+
" std::vector<int>::iterator i= v.begin();\n"
3661+
" if(i == v.end()) { return; }\n"
3662+
" if (*(i+1) == *i) {}\n"
3663+
"}\n");
3664+
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());
3665+
3666+
check("void f(std::vector<int> & v) {\n"
3667+
" std::vector<int>::iterator i= v.begin();\n"
3668+
" if(i != v.end() && (i+1) != v.end() && *(i+1) == *i) {}\n"
3669+
"}\n");
3670+
ASSERT_EQUALS("", errout.str());
3671+
3672+
check("void f(std::string s) {\n"
3673+
" for (std::string::const_iterator i = s.begin(); i != s.end(); ++i) {\n"
3674+
" if (i != s.end() && (i + 1) != s.end() && *(i + 1) == *i) {\n"
3675+
" if (!isalpha(*(i + 2))) {\n"
3676+
" std::string modifier;\n"
3677+
" modifier += *i;\n"
3678+
" modifier += *(i + 1);\n"
3679+
" }\n"
3680+
" }\n"
3681+
" }\n"
3682+
"}\n");
3683+
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());
3684+
3685+
check("void f(std::string s) {\n"
3686+
" for (std::string::const_iterator i = s.begin(); i != s.end(); ++i) {\n"
3687+
" if (i != s.end() && (i + 1) != s.end() && *(i + 1) == *i) {\n"
3688+
" if ((i + 2) != s.end() && !isalpha(*(i + 2))) {\n"
3689+
" std::string modifier;\n"
3690+
" modifier += *i;\n"
3691+
" modifier += *(i + 1);\n"
3692+
" }\n"
3693+
" }\n"
3694+
" }\n"
3695+
"}\n");
3696+
ASSERT_EQUALS("", errout.str());
36433697
}
36443698

36453699
void dereferenceInvalidIterator2() {

0 commit comments

Comments
 (0)