Skip to content

Commit e8ac355

Browse files
PKEuSPKEuS
committed
Refactorized iterator checking:
- Fixed false positive cppcheck-opensource#5669 - Use symboldatabase in CheckStl::pushback() - Improved support for erase on std::vector and find
1 parent 1252c70 commit e8ac355

2 files changed

Lines changed: 121 additions & 102 deletions

File tree

‎lib/checkstl.cpp‎

Lines changed: 73 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ void CheckStl::iterators()
150150
}
151151

152152
// invalidate the iterator if it is erased
153-
else if (tok2->strAt(2) == std::string("erase")) {
153+
else if (tok2->strAt(2) == "erase" && (tok2->strAt(4) != "*" || (container && tok2->varId() == container->declarationId()))) {
154154
validIterator = false;
155155
eraseToken = tok2;
156156
invalidationScope = tok2->scope();
@@ -593,128 +593,100 @@ void CheckStl::pushback()
593593
}
594594

595595
// Iterator becomes invalid after reserve, resize, insert, push_back or push_front..
596-
for (std::size_t i = 0; i < functions; ++i) {
597-
const Scope * scope = symbolDatabase->functionScopes[i];
598-
for (const Token* tok = scope->classStart->next(); tok && tok != scope->classEnd; tok = tok->next()) {
599-
if (!Token::simpleMatch(tok, "vector <"))
600-
continue;
601-
602-
// if iterator declaration inside for() loop
603-
bool iteratorDeclaredInsideLoop = false;
604-
if ((tok->tokAt(-2) && Token::simpleMatch(tok->tokAt(-2), "for (")) ||
605-
(tok->tokAt(-4) && Token::simpleMatch(tok->tokAt(-4), "for ( std ::"))) {
606-
iteratorDeclaredInsideLoop = true;
607-
}
608-
609-
while (tok && tok->str() != ">")
610-
tok = tok->next();
611-
if (!tok)
612-
break;
613-
if (!Token::Match(tok, "> :: iterator|const_iterator %var% =|;"))
614-
continue;
615-
616-
const unsigned int iteratorid(tok->tokAt(3)->varId());
617-
if (iteratorid == 0)
618-
continue;
596+
for (unsigned int iteratorId = 1; iteratorId < symbolDatabase->getVariableListSize(); iteratorId++) {
597+
const Variable* var = symbolDatabase->getVariableFromVarId(iteratorId);
619598

620-
if (iteratorDeclaredInsideLoop && tok->strAt(4) == "=") {
621-
// skip "> :: iterator|const_iterator"
622-
tok = tok->tokAt(3);
623-
}
599+
// Check that its an iterator
600+
if (!var || !var->isLocal() || !Token::Match(var->typeEndToken(), "iterator|const_iterator|reverse_iterator|const_reverse_iterator"))
601+
continue;
624602

625-
// the variable id for the vector
626-
unsigned int vectorid = 0;
603+
// ... on std::vector
604+
if (!Token::Match(var->typeStartToken(), "std| ::| vector <"))
605+
continue;
627606

628-
// count { , } and parentheses for tok2
629-
int indent = 0;
607+
// the variable id for the vector
608+
unsigned int vectorid = 0;
630609

631-
const Token* validatingToken = 0;
610+
const Token* validatingToken = 0;
632611

633-
std::string invalidIterator;
634-
for (const Token *tok2 = tok; indent >= 0 && tok2; tok2 = tok2->next()) {
635-
if (tok2->str() == "{" || tok2->str() == "(")
636-
++indent;
637-
else if (tok2->str() == "}" || tok2->str() == ")") {
638-
if (indent == 0 && Token::simpleMatch(tok2, ") {"))
639-
tok2 = tok2->next();
640-
else
641-
--indent;
642-
}
612+
std::string invalidIterator;
613+
const Token* end2 = var->scope()->classEnd;
614+
for (const Token *tok2 = var->nameToken(); tok2 != end2; tok2 = tok2->next()) {
643615

644-
if (validatingToken == tok2) {
645-
invalidIterator.clear();
646-
validatingToken = 0;
647-
}
616+
if (validatingToken == tok2) {
617+
invalidIterator.clear();
618+
validatingToken = 0;
619+
}
648620

649-
// Using push_back or push_front inside a loop..
650-
if (Token::simpleMatch(tok2, "for (")) {
651-
tok2 = tok2->tokAt(2);
652-
++indent;
653-
}
621+
// Using push_back or push_front inside a loop..
622+
if (Token::simpleMatch(tok2, "for (")) {
623+
tok2 = tok2->tokAt(2);
624+
}
654625

655-
if (Token::Match(tok2, "%varid% = %var% . begin|rbegin|cbegin|crbegin ( ) ; %varid% != %var% . end|rend|cend|crend ( ) ; ++| %varid% ++| ) {", iteratorid)) {
656-
// variable id for the loop iterator
657-
const unsigned int varId(tok2->tokAt(2)->varId());
658-
if (varId == 0)
659-
continue;
626+
if (Token::Match(tok2, "%varid% = %var% . begin|rbegin|cbegin|crbegin ( ) ; %varid% != %var% . end|rend|cend|crend ( ) ; ++| %varid% ++| ) {", iteratorId)) {
627+
// variable id for the loop iterator
628+
const unsigned int varId(tok2->tokAt(2)->varId());
629+
if (varId == 0)
630+
continue;
660631

661-
const Token *pushbackTok = nullptr;
632+
const Token *pushbackTok = nullptr;
662633

663-
// Count { and } for tok3
664-
const Token *tok3 = tok2->tokAt(20);
665-
for (const Token* const end3 = tok3->linkAt(-1); tok3 != end3; tok3 = tok3->next()) {
666-
if (tok3->str() == "break" || tok3->str() == "return") {
667-
pushbackTok = 0;
668-
break;
669-
} else if (Token::Match(tok3, "%varid% . push_front|push_back|insert|reserve|resize|clear (", varId) && !tok3->previous()->isAssignmentOp()) {
634+
// Count { and } for tok3
635+
const Token *tok3 = tok2->tokAt(20);
636+
for (const Token* const end3 = tok3->linkAt(-1); tok3 != end3; tok3 = tok3->next()) {
637+
if (tok3->str() == "break" || tok3->str() == "return") {
638+
pushbackTok = 0;
639+
break;
640+
} else if (Token::Match(tok3, "%varid% . push_front|push_back|insert|reserve|resize|clear|erase (", varId) && !tok3->previous()->isAssignmentOp()) {
641+
if (tok3->strAt(2) != "erase" || (tok3->tokAt(4)->varId() != iteratorId && tok3->tokAt(5)->varId() != iteratorId)) // This case is handled in: CheckStl::iterators()
670642
pushbackTok = tok3->tokAt(2);
671-
}
672643
}
673-
674-
if (pushbackTok)
675-
invalidIteratorError(pushbackTok, pushbackTok->str(), tok2->str());
676644
}
677645

678-
// Assigning iterator..
679-
if (Token::Match(tok2, "%varid% =", iteratorid)) {
680-
if (Token::Match(tok2->tokAt(2), "%var% . begin|end|rbegin|rend|cbegin|cend|crbegin|crend|insert (")) {
681-
if (!invalidIterator.empty() && Token::Match(tok2->tokAt(4), "insert ( %varid% ,", iteratorid)) {
682-
invalidIteratorError(tok2, invalidIterator, tok2->strAt(6));
683-
break;
684-
}
685-
vectorid = tok2->tokAt(2)->varId();
686-
tok2 = tok2->linkAt(5);
687-
} else {
688-
vectorid = 0;
689-
}
690-
invalidIterator = "";
691-
}
646+
if (pushbackTok)
647+
invalidIteratorError(pushbackTok, pushbackTok->str(), tok2->str());
648+
}
692649

693-
// push_back on vector..
694-
if (vectorid > 0 && Token::Match(tok2, "%varid% . push_front|push_back|insert|reserve|resize|clear (", vectorid)) {
695-
if (!invalidIterator.empty() && Token::Match(tok2->tokAt(2), "insert ( %varid% ,", iteratorid)) {
696-
invalidIteratorError(tok2, invalidIterator, tok2->strAt(4));
650+
// Assigning iterator..
651+
if (Token::Match(tok2, "%varid% =", iteratorId)) {
652+
if (Token::Match(tok2->tokAt(2), "%var% . begin|end|rbegin|rend|cbegin|cend|crbegin|crend|insert|erase|find (")) {
653+
if (!invalidIterator.empty() && Token::Match(tok2->tokAt(4), "insert|erase ( *| %varid% )|,", iteratorId)) {
654+
invalidIteratorError(tok2, invalidIterator, var->name());
697655
break;
698656
}
657+
vectorid = tok2->tokAt(2)->varId();
658+
tok2 = tok2->linkAt(5);
659+
} else {
660+
vectorid = 0;
661+
}
662+
invalidIterator = "";
663+
}
699664

700-
invalidIterator = tok2->strAt(2);
701-
tok2 = tok2->linkAt(3);
665+
// push_back on vector..
666+
if (vectorid > 0 && Token::Match(tok2, "%varid% . push_front|push_back|insert|reserve|resize|clear|erase (", vectorid)) {
667+
if (!invalidIterator.empty() && Token::Match(tok2->tokAt(2), "insert|erase ( *| %varid% ,|)", iteratorId)) {
668+
invalidIteratorError(tok2, invalidIterator, var->name());
669+
break;
702670
}
703671

704-
else if (tok2->str() == "return" || tok2->str() == "throw")
705-
validatingToken = Token::findsimplematch(tok2->next(), ";");
672+
if (tok2->strAt(2) != "erase" || (tok2->tokAt(4)->varId() != iteratorId && tok2->tokAt(5)->varId() != iteratorId)) // This case is handled in: CheckStl::iterators()
673+
invalidIterator = tok2->strAt(2);
674+
tok2 = tok2->linkAt(3);
675+
}
706676

707-
// TODO: instead of bail out for 'else' try to check all execution paths.
708-
else if (tok2->str() == "break" || tok2->str() == "else")
709-
invalidIterator.clear();
677+
else if (tok2->str() == "return" || tok2->str() == "throw")
678+
validatingToken = Token::findsimplematch(tok2->next(), ";");
710679

711-
// Using invalid iterator..
712-
if (!invalidIterator.empty()) {
713-
if (Token::Match(tok2, "++|--|*|+|-|(|,|=|!= %varid%", iteratorid))
714-
invalidIteratorError(tok2, invalidIterator, tok2->strAt(1));
715-
if (Token::Match(tok2, "%varid% ++|--|+|-|.", iteratorid))
716-
invalidIteratorError(tok2, invalidIterator, tok2->str());
717-
}
680+
// TODO: instead of bail out for 'else' try to check all execution paths.
681+
else if (tok2->str() == "break" || tok2->str() == "else")
682+
invalidIterator.clear();
683+
684+
// Using invalid iterator..
685+
if (!invalidIterator.empty()) {
686+
if (Token::Match(tok2, "++|--|*|+|-|(|,|=|!= %varid%", iteratorId))
687+
invalidIteratorError(tok2, invalidIterator, tok2->strAt(1));
688+
if (Token::Match(tok2, "%varid% ++|--|+|-|.", iteratorId))
689+
invalidIteratorError(tok2, invalidIterator, tok2->str());
718690
}
719691
}
720692
}

‎test/teststl.cpp‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ class TestStl : public TestFixture {
7272
TEST_CASE(eraseAssignByFunctionCall);
7373
TEST_CASE(eraseErase);
7474
TEST_CASE(eraseByValue);
75+
TEST_CASE(eraseOnVector);
7576

7677
TEST_CASE(pushback1);
7778
TEST_CASE(pushback2);
@@ -85,7 +86,6 @@ class TestStl : public TestFixture {
8586
TEST_CASE(pushback10);
8687
TEST_CASE(pushback11);
8788
TEST_CASE(pushback12);
88-
8989
TEST_CASE(insert1);
9090
TEST_CASE(insert2);
9191

@@ -984,9 +984,56 @@ class TestStl : public TestFixture {
984984
" foo.erase(*it);\n"
985985
"}");
986986
ASSERT_EQUALS("", errout.str());
987+
988+
// #5669
989+
check("void f() {\n"
990+
" HashSet_Ref::iterator aIt = m_ImplementationMap.find( xEle );\n"
991+
" m_SetLoadedFactories.erase(*aIt);\n"
992+
" m_SetLoadedFactories.erase(aIt);\n"
993+
"}");
994+
ASSERT_EQUALS("", errout.str());
995+
996+
check("void f(const std::list<int>& m_ImplementationMap) {\n"
997+
" std::list<int>::iterator aIt = m_ImplementationMap.find( xEle );\n"
998+
" m_ImplementationMap.erase(*aIt);\n"
999+
" m_ImplementationMap.erase(aIt);\n"
1000+
"}");
1001+
ASSERT_EQUALS("[test.cpp:4]: (error) Invalid iterator: aIt\n", errout.str());
1002+
1003+
check("void f(const std::list<int>& m_ImplementationMap) {\n"
1004+
" std::list<int>::iterator aIt = m_ImplementationMap.find( xEle1 );\n"
1005+
" std::list<int>::iterator bIt = m_ImplementationMap.find( xEle2 );\n"
1006+
" m_ImplementationMap.erase(*bIt);\n"
1007+
" m_ImplementationMap.erase(aIt);\n"
1008+
"}");
1009+
ASSERT_EQUALS("", errout.str());
9871010
}
9881011

9891012

1013+
void eraseOnVector() {
1014+
check("void f(const std::vector<int>& m_ImplementationMap) {\n"
1015+
" std::vector<int>::iterator aIt = m_ImplementationMap.find( xEle );\n"
1016+
" m_ImplementationMap.erase(something(unknown));\n" // All iterators become invalidated when erasing from std::vector
1017+
" m_ImplementationMap.erase(aIt);\n"
1018+
"}");
1019+
ASSERT_EQUALS("[test.cpp:4]: (error) After erase(), the iterator 'aIt' may be invalid.\n", errout.str());
1020+
1021+
check("void f(const std::vector<int>& m_ImplementationMap) {\n"
1022+
" std::vector<int>::iterator aIt = m_ImplementationMap.find( xEle );\n"
1023+
" m_ImplementationMap.erase(*aIt);\n" // All iterators become invalidated when erasing from std::vector
1024+
" m_ImplementationMap.erase(aIt);\n"
1025+
"}");
1026+
ASSERT_EQUALS("[test.cpp:4]: (error) Invalid iterator: aIt\n", errout.str());
1027+
1028+
check("void f(const std::vector<int>& m_ImplementationMap) {\n"
1029+
" std::vector<int>::iterator aIt = m_ImplementationMap.find( xEle1 );\n"
1030+
" std::vector<int>::iterator bIt = m_ImplementationMap.find( xEle2 );\n"
1031+
" m_ImplementationMap.erase(*bIt);\n" // All iterators become invalidated when erasing from std::vector
1032+
" aIt = m_ImplementationMap.erase(aIt);\n"
1033+
"}");
1034+
ASSERT_EQUALS("[test.cpp:5]: (error) After erase(), the iterator 'aIt' may be invalid.\n", errout.str());
1035+
}
1036+
9901037
void pushback1() {
9911038
check("void f(const std::vector<int> &foo)\n"
9921039
"{\n"

0 commit comments

Comments
 (0)