{{ message }}
Shared: Support flow summaries from ReturnValues#22061
Open
MathiasVP wants to merge 16 commits into
Open
Conversation
… from the return value of a call.
github-actions
Bot
added
C#
JS
C++
Java
Python
Go
Ruby
Rust
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Extends the shared flow-summary infrastructure to support “reverse-flow” style summaries where writes through an indirect ReturnValue[*] can be modeled as flowing back into parameters/receiver state (e.g., C++ std::string::operator[]-style patterns). This enables replacing legacy reverse-flow models with flow summaries like ReturnValue[*] -> Argument[this].
Changes:
- Adds shared support for flow summaries originating from return kinds via
FlowSummaryCallBase, plus plumbing to map return-kinds to synthetic parameter positions. - Updates multiple language-specific flow-summary integrations to the new
summaryLocalStep(Node pred, SummaryNode succ, ...)API and addsStepsInputSig.getSummaryNode. - Adds/updates C++ external-model tests and models to exercise reverse-flow via
ReturnValue[*].
Show a summary per file
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Low
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
std::stringclass in C++ is mutable so you can do:This is just a regular assignment where the left-hand side is a reference to the internal buffer. Historically, to support this in C++ taint-tracking we've specified "reverse flows":
codeql/cpp/ql/lib/semmle/code/cpp/models/implementations/StdString.qll
Lines 384 to 386 in b8c78fd
So when an assignment flows from the right-hand side to the left-hand side (i.e., to the returned reference (actually to the indirections of the reference, but nevermind)) the "reverse flow" model transfers flow to the qualifier.
We'd obviously like to get rid of all these old models and replace them with flow summaries. However, flow summaries do not allow for these reverse flow summaries. This PR fixes that so that we can model this using a summary such as
ReturnValue[*] -> Argument[this].Most of the work is in bb2ec12. We add a new summary node (which is expected to be lifted to a proper
ArgumentNodeby each language) along with a new parameter/argument position to transfer the value into the summarized callable. 662f522 shows how the new predicates are instantiated for C/C++.Commit-by-commit review recommended.
This PR doesn't actually add any real models to C/C++. However, I have a local branch where I've checked on DCA that the performance of these changes are fine.