Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #11093, #12377 FP mismatchingContainerIterator #5911

Merged
merged 6 commits into from
Feb 9, 2024
+69 −6
Merged

Fix #11093, #12377 FP mismatchingContainerIterator #5911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
364a8c6
Fix #11093 FP: mismatchingContainerIterator caused by ternary operato…
chrchr-github Jan 23, 2024
f97a804
Fix #12377 FP mismatchingContainerIterator with pointer to vector
chrchr-github Jan 23, 2024
3dd78b5
Use stl algo
chrchr-github Jan 23, 2024
dcbd0dc
Add test
chrchr-github Jan 25, 2024
48b4406
Merge branch 'main' into chr_Fix11093
chrchr-github Jan 29, 2024
0a22135
Fix
chrchr-github Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions lib/checkstl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,11 +686,19 @@ void CheckStl::sameIteratorExpressionError(const Token *tok)
reportError(tok, Severity::style, "sameIteratorExpression", "Same iterators expression are used for algorithm.", CWE664, Certainty::normal);
}

static const Token* getAddressContainer(const Token* tok)
static std::vector<const Token*> getAddressContainer(const Token* tok)
{
if (Token::simpleMatch(tok, "[") && tok->astOperand1())
return tok->astOperand1();
return tok;
return { tok->astOperand1() };
std::vector<ValueFlow::Value> values = ValueFlow::getLifetimeObjValues(tok, /*inconclusive*/ false);
std::vector<const Token*> res;
for (const auto& v : values) {
if (v.tokvalue)
res.emplace_back(v.tokvalue);
}
if (res.empty())
res.emplace_back(tok);
return res;
}

static bool isSameIteratorContainerExpression(const Token* tok1,
Expand All @@ -701,10 +709,18 @@ static bool isSameIteratorContainerExpression(const Token* tok1,
if (isSameExpression(true, false, tok1, tok2, library, false, false)) {
return !astIsContainerOwned(tok1) || !isTemporary(true, tok1, &library);
}
if (kind == ValueFlow::Value::LifetimeKind::Address) {
return isSameExpression(true, false, getAddressContainer(tok1), getAddressContainer(tok2), library, false, false);
if (astContainerYield(tok2) == Library::Container::Yield::ITEM)
return true;
if (kind == ValueFlow::Value::LifetimeKind::Address || kind == ValueFlow::Value::LifetimeKind::Iterator) {
const auto address1 = getAddressContainer(tok1);
const auto address2 = getAddressContainer(tok2);
return std::any_of(address1.begin(), address1.end(), [&](const Token* tok1) {
return std::any_of(address2.begin(), address2.end(), [&](const Token* tok2) {
return isSameExpression(true, false, tok1, tok2, library, false, false);
});
});
}
return astContainerYield(tok2) == Library::Container::Yield::ITEM;
return false;
}

static ValueFlow::Value getLifetimeIteratorValue(const Token* tok, MathLib::bigint path = 0)
Expand Down Expand Up @@ -866,6 +882,8 @@ void CheckStl::mismatchingContainerIterator()
ValueFlow::Value val = getLifetimeIteratorValue(iterTok);
if (!val.tokvalue)
continue;
if (!val.isKnown() && Token::simpleMatch(val.tokvalue->astParent(), ":"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lifetime values are never known. Also, this will lead to FNs as well. I think it would be better to check that the set of lifetimes are the same when there are multiple lifetimes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the problem that there is only one value? Otherwise, getLifetimeIteratorValue() would return an empty value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yea, thats right, but it should be multiple values. I wonder why we are not getting both lifetime.

I assumed the error is that one variable has the lifetime v1 and the other has the lifetime v2. It should have both lifetimes.

continue;
if (val.lifetimeKind != ValueFlow::Value::LifetimeKind::Iterator)
continue;
if (iterTok->str() == "*" && iterTok->astOperand1()->valueType() && iterTok->astOperand1()->valueType()->type == ValueType::ITERATOR)
Expand Down
45 changes: 45 additions & 0 deletions test/teststl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2136,6 +2136,51 @@ class TestStl : public TestFixture {
"}\n");
ASSERT_EQUALS("", errout.str());

// #11093
check("struct S {\n"
" std::vector<int> v1, v2;\n"
" void f(bool b) {\n"
" std::vector<int>& v = b ? v1 : v2;\n"
" v.erase(v.begin());\n"
" }\n"
"};\n");
ASSERT_EQUALS("", errout.str());

// #12377
check("void f(bool b) {\n"
" std::vector<int> *pv;\n"
" if (b) {\n"
" std::vector<int>& r = get1();\n"
" pv = &r;\n"
" }\n"
" else {\n"
" std::vector<int>& r = get2();\n"
" pv = &r;\n"
" }\n"
" std::vector<int>::iterator it = pv->begin();\n"
" it = pv->erase(it, it + 2);\n"
"}\n");
ASSERT_EQUALS("", errout.str());

check("struct S {\n"
" std::vector<int> v;\n"
" void f() {\n"
" std::vector<int>* p = &v;\n"
" p->insert(std::find(p->begin(), p->end(), 0), 1);\n"
" }\n"
"};\n");
ASSERT_EQUALS("", errout.str());

check("struct S {\n"
" std::vector<int> v;\n"
" void f(int i) {\n"
" std::vector<int>* p = &v;\n"
" if (p->size() > i)\n"
" p->erase(p->begin() + i, p->end());\n"
" }\n"
"};\n");
ASSERT_EQUALS("", errout.str());

// #11067
check("struct S {\n"
" std::vector<int> v;\n"
Expand Down
Loading