C++: IR Alias Analysis for smart pointers#5737
Merged
MathiasVP merged 12 commits intogithub:mainfrom Apr 27, 2021
dbartol:dbartol/smart-pointers/work
Merged
C++: IR Alias Analysis for smart pointers#5737MathiasVP merged 12 commits intogithub:mainfrom dbartol:dbartol/smart-pointers/work
MathiasVP merged 12 commits intogithub:mainfrom
dbartol:dbartol/smart-pointers/work
Conversation
added 8 commits
April 20, 2021 14:03
MathiasVP
reviewed
Apr 21, 2021
Contributor
MathiasVP
left a comment
There was a problem hiding this comment.
Looks great! I mostly have nitpicky comments.
cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll
Show resolved
Hide resolved
cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll
Outdated
Show resolved
Hide resolved
cpp/ql/src/semmle/code/cpp/models/implementations/SmartPointer.qll
Outdated
Show resolved
Hide resolved
added 2 commits
April 21, 2021 12:12
`DataFlowFunction` models treat references a pointers - an explicit level of indirection. The AST dataflow library generally treats references as if they were the referred-to object. This commit removes a workaround in the dataflow model for unary `operator*` on smart pointers, and makes the AST dataflow library adjust the results of querying the model so that a returned reference only gets flow that was modeled as going to the dereference of the return value. This fixes some missing flow in IR dataflow, and recovers some (presumably) missing reverse taint flow in AST taint tracking as well.
MathiasVP
reviewed
Apr 22, 2021
added 2 commits
April 22, 2021 16:51
Smart pointer constructors, assignments, and `reset()` can actually have fairly large side effects, especially with custom deleters, destructors for objects being destroyed, and so on. I've re-introduced `{AllAliased}` side effects for these functions. There was no immediate effect on analysis results.
Contributor
|
I've restarted the CPP-differences run: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1945/ |
Contributor
|
https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1948 failed again with an error: I see that 97a13c9 is the commit that's being compared in the CPP-differences run, but it's not the HEAD of this PR (which might be why the job is failing). I've started another CPP-differences that compares the latest-merged |
Author
|
CPP-Differences shows no changes, and I didn't really expect any. I think we're good. |
Merged
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
3 participants
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.
This PR brings IR alias analysis of smart pointers more-or-less up to parity with the analysis of raw pointers, at least when building aliased SSA. I recommend reviewing commit-by-commit.
CallInstruction::getAParameterSideEffect()to get the side effects for the specified parameter index.memory.hinto a sharedincludedirectory in the test tree, and added the necessary parts ofutility.handtype_traits.has well.AliasFunctionmodel to allow specifying pointer flow between arbitrary inputs and outputs. Note that pointer flow differs from data flow in that pointer flow is "must" flow, but data flow is "may" flow. We may want to consider consolidating these into a single model with abooleanparameter someday.std::weak_ptrtoString()of dynamic allocations stable, so that they can be used in inline expectations.There are still some taint test failures, which are caused because I stopped excluding
operator*()from the list of modeled unwrapper functions. This is being discussed on Slack.