Skip to content

C++: Fix FPs on cpp/invalid-pointer-deref#10593

Merged
rdmarsh2 merged 2 commits into
github:mainfrom
MathiasVP:fix-fp-on-cwe-193
Sep 27, 2022
Merged

C++: Fix FPs on cpp/invalid-pointer-deref#10593
rdmarsh2 merged 2 commits into
github:mainfrom
MathiasVP:fix-fp-on-cwe-193

Conversation

@MathiasVP

@MathiasVP MathiasVP commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

It's super difficult to properly fix the FP I added in #10572. So for now I've decided to just block all flow through back-edges of phi nodes (as suggested by @aschackmull) since this one one of the things that had to be present in order to get an FP here.

@MathiasVP MathiasVP requested a review from a team as a code owner September 27, 2022 13:52
@github-actions github-actions Bot added the C++ label Sep 27, 2022
*/
final Node getAnInput(boolean fromBackEdge) {
localFlowStep(result, this) and
if phi.getBasicBlock().dominates(getBasicBlock(result))

Check warning

Code scanning / CodeQL

QL-for-QL encountered an internal consistency error

PredConsistency::multipleResolveCall
Comment thread cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll Fixed
Comment thread cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll Fixed
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Sep 27, 2022
@MathiasVP MathiasVP requested a review from rdmarsh2 September 27, 2022 20:41
pragma[nomagic]
predicate pointerAddInstructionHasBounds(
PointerAddInstruction pai, DataFlow::Node sink1, Instruction sink2, int delta
PointerAddInstruction pai, DataFlow::Node sink1, DataFlow::Node sink2, int delta

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.

Why this change? Just a performance thing or is there some semantic difference I'm missing?

@MathiasVP MathiasVP Sep 27, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. Sorry. That was part of a solution that I ended up discarding. But I kept it in the PR since I liked the consistency of having both sinks be nodes instead of one being passed as a node and another being passed as an instruction.

So no, there is no behavior change from this at all.

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.

3 participants