Skip to content

Fix #11553 pop_back on empty container is UB#4789

Merged
firewave merged 13 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11553
Feb 24, 2023
Merged

Fix #11553 pop_back on empty container is UB#4789
firewave merged 13 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11553

Conversation

@chrchr-github
Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread lib/checkstl.cpp Outdated
!containerAppendsElement(container, parent)))) {
if (value.intvalue == 0 && (indexTok ||
(containerYieldsElement(container, parent) && !containerAppendsElement(container, parent)) ||
(value.isKnown() && containerPopsElement(container, parent)))) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should be able to warn when the value is not known as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The problem is code like

void f(std::vector<int>& v) {
	v.pop_back();
	if (v.empty()) {}
}

We don't seem to take into account that the size changes with pop_back().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That looks like a bug in Valueflow. Maybe we don't account for the POP action.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Feb 13, 2023

Choose a reason for hiding this comment

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

It's not necessarily a ValueFlow issue.
Propagating possible values to locations preceding a size-changing action seems wrong.

We don't warn for this either, although v.resize() does get a possible empty value:

void g(std::vector<int>& v) {
	int n = v.size();
	v.resize(n - 1);
	if (v.empty()) {}
}

containerYieldsElement() is obviously false, and containerAppendsElement() does not check whether resize() adds or removes elements.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We also don't warn for

void g(std::vector<int>& v) {
	v.push_back(1);
	if (v.empty()) {}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So I fixed the valueflow issues in #4800 and #4803 so you should be able to warn for possible values as well.

For the v.resize(n - 1) case, the checker would need to be updated, but I dont think it needs done in this PR though.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nice. So we should be able to warn for

void f(std::vector<int>& v) {
	if (v.empty()) {}
	v.pop_back();
}

after those PRs have been merged.
The resize(n - 1) example is equivalent to the pop_back() one in the 2nd comment above, so we shouldn't warn for those.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think #4800 is only needed to support possible values as #4803 needs some more work to fix FPs.

Comment thread test/cfg/std.cpp Outdated
@firewave firewave merged commit 50c8a0d into cppcheck-opensource:main Feb 24, 2023
@chrchr-github chrchr-github deleted the chr_Fix11553 branch March 13, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants