Skip to content

Commit ce2ff87

Browse files
KiritoYGKiritoYG
committed
Fix invalid iterator warning for member-backed arrow access
1 parent 8bb772d commit ce2ff87

2 files changed

Lines changed: 212 additions & 2 deletions

File tree

‎lib/checkstl.cpp‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,38 @@ static bool isIterator(const Variable *var, bool& inconclusiveType)
427427
return true;
428428
}
429429

430+
static bool iteratorArrowReturnsMemberAddress(const Variable* var)
431+
{
432+
const Scope* scope = var->typeScope();
433+
if (!scope || !var->type()->derivedFrom.empty())
434+
return false;
435+
const auto operators = scope->functionMap.equal_range("operator->");
436+
if (operators.first == operators.second)
437+
return false;
438+
439+
// Do not assume which cv/ref-qualified overload is selected.
440+
for (auto it = operators.first; it != operators.second; ++it) {
441+
const Function* function = it->second;
442+
if (!function->functionScope || !Function::returnsPointer(function))
443+
return false;
444+
const Token* body = function->functionScope->bodyStart;
445+
if (!Token::simpleMatch(body, "{ return &"))
446+
return false;
447+
const Token* memberToken = body->tokAt(3);
448+
if (Token::simpleMatch(memberToken, "this ."))
449+
memberToken = memberToken->tokAt(2);
450+
if (!Token::Match(memberToken, "%var% ; }") || memberToken->tokAt(2) != function->functionScope->bodyEnd)
451+
return false;
452+
const Variable* member = memberToken->variable();
453+
if (!member || member->scope() != scope || !member->isMember() || member->isStatic() ||
454+
member->isPointer() || member->isReference() || member->isRValueReference())
455+
return false;
456+
if (member->type() && member->type()->getFunction("operator&"))
457+
return false;
458+
}
459+
return true;
460+
}
461+
430462
static std::string getContainerName(const Token *containerToken)
431463
{
432464
if (!containerToken)
@@ -455,6 +487,20 @@ void CheckStlImpl::iterators()
455487

456488
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
457489

490+
// A free or friend unary operator& can change the meaning of returning &member.
491+
bool hasNonMemberAddressOperator = false;
492+
for (const Scope& scope : symbolDatabase->scopeList) {
493+
const auto operators = scope.functionMap.equal_range("operator&");
494+
for (auto it = operators.first; it != operators.second; ++it) {
495+
if (it->second->argCount() == 1 && (it->second->isFriend() || !scope.isClassOrStructOrUnion())) {
496+
hasNonMemberAddressOperator = true;
497+
break;
498+
}
499+
}
500+
if (hasNonMemberAddressOperator)
501+
break;
502+
}
503+
458504
// Filling map of iterators id and their scope begin
459505
std::map<int, const Token*> iteratorScopeBeginInfo;
460506
for (const Variable* var : symbolDatabase->variableList()) {
@@ -615,7 +661,10 @@ void CheckStlImpl::iterators()
615661
dereferenceErasedError(eraseToken, tok2, tok2->strAt(1), inconclusiveType);
616662
tok2 = tok2->next();
617663
} else if (!validIterator && Token::Match(tok2, "%varid% . %name%", iteratorId)) {
618-
dereferenceErasedError(eraseToken, tok2, tok2->str(), inconclusiveType);
664+
// A known operator-> can expose the iterator object's own storage before assignment.
665+
if (eraseToken || !inconclusiveType || tok2->next()->originalName() != "->" || hasNonMemberAddressOperator ||
666+
!iteratorArrowReturnsMemberAddress(var))
667+
dereferenceErasedError(eraseToken, tok2, tok2->str(), inconclusiveType);
619668
tok2 = tok2->tokAt(2);
620669
}
621670

‎test/teststl.cpp‎

Lines changed: 162 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,8 @@ class TestStl : public TestFixture {
164164

165165
TEST_CASE(dereferenceInvalidIterator);
166166
TEST_CASE(dereferenceInvalidIterator2); // #6572
167+
TEST_CASE(dereferenceSelfContainedIterator);
168+
TEST_CASE(dereferenceSelfContainedIteratorOperators);
167169
TEST_CASE(dereference_auto);
168170

169171
TEST_CASE(loopAlgoElementAssign);
@@ -5779,7 +5781,7 @@ class TestStl : public TestFixture {
57795781
" it->m_place = 0;\n"
57805782
" return it;\n"
57815783
"}\n", dinit(CheckOptions, $.inconclusive = true));
5782-
ASSERT_EQUALS("[test.cpp:18:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5784+
ASSERT_EQUALS("", errout_str());
57835785

57845786
check("int f(const std::vector<int>& v) {\n" // #11895
57855787
" auto it = v.end();\n"
@@ -5789,6 +5791,165 @@ class TestStl : public TestFixture {
57895791
ASSERT_EQUALS("", errout_str());
57905792
}
57915793

5794+
void dereferenceSelfContainedIterator() {
5795+
check("struct Value { int field; };\n"
5796+
"struct iterator {\n"
5797+
" Value item;\n"
5798+
" Value& operator*() { return item; }\n"
5799+
" Value* operator->() { return &this->item; }\n"
5800+
" const Value* operator->() const { return &item; }\n"
5801+
" iterator& operator++();\n"
5802+
"};\n"
5803+
"iterator f() {\n"
5804+
" iterator it;\n"
5805+
" it->field = 0;\n"
5806+
" return it;\n"
5807+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5808+
ASSERT_EQUALS("", errout_str());
5809+
5810+
// Safe arrow access must not validate a different dereference operator.
5811+
check("struct Value { int field; };\n"
5812+
"struct iterator {\n"
5813+
" Value item; Value* ptr;\n"
5814+
" Value& operator*() { return *ptr; }\n"
5815+
" Value* operator->() { return &item; }\n"
5816+
" iterator& operator++();\n"
5817+
"};\n"
5818+
"void f() {\n"
5819+
" iterator it;\n"
5820+
" it->field = 0;\n"
5821+
" *it;\n"
5822+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5823+
ASSERT_EQUALS("[test.cpp:11:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5824+
5825+
check("struct Value { int field; };\n"
5826+
"struct iterator {\n"
5827+
" Value* ptr;\n"
5828+
" Value& operator*() { return *ptr; }\n"
5829+
" Value* operator->() { return ptr; }\n"
5830+
" iterator& operator++();\n"
5831+
"};\n"
5832+
"void f() {\n"
5833+
" iterator it;\n"
5834+
" it->field = 0;\n"
5835+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5836+
ASSERT_EQUALS("[test.cpp:10:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5837+
5838+
// One safe overload is insufficient when another overload has unknown semantics.
5839+
check("struct Value { int field; };\n"
5840+
"struct iterator {\n"
5841+
" Value item; Value* ptr;\n"
5842+
" Value& operator*() { return *ptr; }\n"
5843+
" Value* operator->() { return &item; }\n"
5844+
" const Value* operator->() const;\n"
5845+
" iterator& operator++();\n"
5846+
"};\n"
5847+
"void f() {\n"
5848+
" iterator it;\n"
5849+
" it->field = 0;\n"
5850+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5851+
ASSERT_EQUALS("[test.cpp:11:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5852+
5853+
// The address-of operator can itself be overloaded.
5854+
check("struct Value { int field; Value* operator&(); };\n"
5855+
"struct iterator {\n"
5856+
" Value item;\n"
5857+
" Value& operator*() { return item; }\n"
5858+
" Value* operator->() { return &item; }\n"
5859+
" iterator& operator++();\n"
5860+
"};\n"
5861+
"void f() {\n"
5862+
" iterator it;\n"
5863+
" it->field = 0;\n"
5864+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5865+
ASSERT_EQUALS("[test.cpp:10:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5866+
5867+
// Keep the existing invalidation heuristic after an erase operation.
5868+
check("struct Value { int field; };\n"
5869+
"struct iterator {\n"
5870+
" Value item;\n"
5871+
" Value& operator*() { return item; }\n"
5872+
" Value* operator->() { return &item; }\n"
5873+
" iterator& operator++();\n"
5874+
"};\n"
5875+
"struct Container { void erase(iterator); };\n"
5876+
"void f(Container& c) {\n"
5877+
" iterator it{};\n"
5878+
" c.erase(it);\n"
5879+
" it->field = 0;\n"
5880+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5881+
ASSERT_EQUALS("[test.cpp:12:5] -> [test.cpp:11:5]: (error, inconclusive) Iterator 'it' used after element has been erased. [eraseDereference]\n", errout_str());
5882+
}
5883+
5884+
void dereferenceSelfContainedIteratorOperators() {
5885+
// free address of
5886+
check("struct Value { int field; };\n"
5887+
"Value* operator&(Value&) { return nullptr; }\n"
5888+
"struct iterator {\n"
5889+
" Value item;\n"
5890+
" Value& operator*() { return item; }\n"
5891+
" Value* operator->() { return &item; }\n"
5892+
" iterator& operator++();\n"
5893+
"};\n"
5894+
"void f() {\n"
5895+
" iterator it;\n"
5896+
" it->field = 0;\n"
5897+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5898+
ASSERT_EQUALS("[test.cpp:11:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5899+
5900+
// inherited arrow
5901+
check("struct Value { int field; };\n"
5902+
"struct Base {\n"
5903+
" Value* operator->() const { return nullptr; }\n"
5904+
"};\n"
5905+
"struct iterator : Base {\n"
5906+
" using Base::operator->;\n"
5907+
" Value item;\n"
5908+
" iterator() {}\n"
5909+
" Value& operator*() { return item; }\n"
5910+
" Value* operator->() { return &item; }\n"
5911+
" iterator& operator++();\n"
5912+
"};\n"
5913+
"void f() {\n"
5914+
" const iterator it;\n"
5915+
" it->field = 0;\n"
5916+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5917+
ASSERT_EQUALS("[test.cpp:15:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5918+
5919+
// arrow proxy
5920+
check("struct Value { int field; };\n"
5921+
"struct Proxy {\n"
5922+
" Proxy(Value*) {}\n"
5923+
" Value* operator->() { return nullptr; }\n"
5924+
"};\n"
5925+
"struct iterator {\n"
5926+
" Value item;\n"
5927+
" Value& operator*() { return item; }\n"
5928+
" Proxy operator->() { return &item; }\n"
5929+
" iterator& operator++();\n"
5930+
"};\n"
5931+
"void f() {\n"
5932+
" iterator it;\n"
5933+
" it->field = 0;\n"
5934+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5935+
ASSERT_EQUALS("[test.cpp:14:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5936+
5937+
// friend unary address of overload
5938+
check("struct Value { int field; friend Value* operator&(Value&) { return nullptr; } };\n"
5939+
"struct iterator {\n"
5940+
" Value item;\n"
5941+
" Value& operator*() { return item; }\n"
5942+
" Value* operator->() { return &item; }\n"
5943+
" iterator& operator++();\n"
5944+
"};\n"
5945+
"void f() {\n"
5946+
" iterator it;\n"
5947+
" it->field = 0;\n"
5948+
"}\n", dinit(CheckOptions, $.inconclusive = true));
5949+
ASSERT_EQUALS("[test.cpp:10:5]: (error, inconclusive) Invalid iterator 'it' used. [eraseDereference]\n", errout_str());
5950+
5951+
}
5952+
57925953
void loopAlgoElementAssign() {
57935954
check("void foo() {\n"
57945955
" for(int& x:v)\n"

0 commit comments

Comments
 (0)