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.

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

C++: Iterator derefs are partial writes #18674

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Feb 4, 2025

In #15633 we started to interpret flow out of a non-MaD dataflow/taint-flow model (i.e., a class that extends DataFlowFunction or TaintFlowFunction) as totally overwriting the destination buffer.

Obviously, since not all functions overwrite the entire destination buffer, we had to come up with a way to opt out of this now-default behavior. So we added an isPartialWrite predicate that could be overwritten on classes that did not overwrite the entire buffer. In #15633 I went through most of our classes to add this, but I see that I missed the operator* functions in iterators.

Why do operator* even have flow out, you may ask? That's a good question! It stems from a pretty neat idea we had back in the days of AST dataflow that we could provide flow from source() to myIterator in:

*myIterator = source()

(which would then have another step to its underlying container) by adding a dataflow model for operator* from the return value to the qualifier. That is, the opposite direction of what you'd normally add for flow out of operator* (the same idea is used in dataflow)

The end result is that operator* actually does "write" to its qualifier, and this write is obviously not meant to totally overwrite the qualifier.

For various reasons, it's not actually possible to observe any missing flow because of this, but I'm currently working on another PR where this matters.

@Copilot Copilot bot review requested due to automatic review settings February 4, 2025 13:33
@MathiasVP MathiasVP requested a review from a team as a code owner February 4, 2025 13:33

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@github-actions github-actions bot added the C++ label Feb 4, 2025
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Feb 4, 2025
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM if DCA is happy.

@MathiasVP
Copy link
Contributor Author

Hm. The DCA results looks funny. It seems like we have no changes in the number of results, but cpp/overrun-write now produces waaaay larger BQRs on a single project 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants