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++: Fix some FPs in cpp/missing-check-scanf (second attempt) #17938

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 8, 2024

#16009 gave good results, but was unfortunately closed due to performance problems in DCA. Initially, I had concluded that GVN was causing ensuresEq(test, e, ..., block, ...) to hold for too many (e, block) pairs and thus causing a slowdown.

After working through some performance problems in Ben's alternative PR #17907 I realized that the performance problem was actually coming from an existing problem in the unary_simple_comparison_eq predicate: currently, on main, there is a very large QLDoc and some very ad-hoc conjunctions in the second disjunct of unary_simple_comparison_eq to reduce the size of the predicate. However, it turns out that this is too doing a very good job.

Instead, the second commit is restricting the predicate properly by pruning the set down to only those tests that are used in unary comparisons that we actually care about.

Commits a40c1d5 and db38069 are taken from #16009 which @jketema has already looked at (although I don't blame you for wanting to re-review it if you want to. Let me know if you want me to split up the complex commit again like in #16009). The only new thing is the pruning in 442968c

The commits that were tested at the latest DCA is consistent with the HEAD of this branch. I just force-pushed to clean up tests remove some spurious imports I had when experimenting with stuff.

@github-actions github-actions bot added the C++ label Nov 8, 2024
@MathiasVP MathiasVP force-pushed the fix-fp-in-missing-check-scanf-fixing-take-2 branch 2 times, most recently from e0f0b0a to 930efc2 Compare November 11, 2024 11:22
@MathiasVP
Copy link
Contributor Author

MathiasVP commented Nov 13, 2024

Bahh.... WAY too many join orders are broken by this and I don't feel like fixing them all 😪 I'll go back to keeping the non-GVN interface Done!

@MathiasVP MathiasVP force-pushed the fix-fp-in-missing-check-scanf-fixing-take-2 branch 4 times, most recently from d01d8f1 to 7a802d5 Compare November 14, 2024 14:16
@MathiasVP MathiasVP force-pushed the fix-fp-in-missing-check-scanf-fixing-take-2 branch from 7a802d5 to ac2630c Compare November 14, 2024 16:20
@MathiasVP MathiasVP force-pushed the fix-fp-in-missing-check-scanf-fixing-take-2 branch from ac2630c to bb85aa2 Compare November 14, 2024 16:30
@MathiasVP MathiasVP marked this pull request as ready for review November 14, 2024 16:33
@MathiasVP MathiasVP requested a review from a team as a code owner November 14, 2024 16:33
@jketema jketema changed the title C++: #16009 (second attempt) C++: Fix some FPs in cpp/missing-check-scanf (second attempt) Nov 14, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 14, 2024
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.

Comments on the first two commits. There's a few comments to occur in several places. Feel free to only react to one of those if you think nothing needs to be changed.

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved
cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved
cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved
cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved
MathiasVP and others added 3 commits November 14, 2024 21:28
Co-authored-by: Jeroen Ketema <[email protected]>
Co-authored-by: Jeroen Ketema <[email protected]>
Co-authored-by: Jeroen Ketema <[email protected]>
@MathiasVP MathiasVP force-pushed the fix-fp-in-missing-check-scanf-fixing-take-2 branch from 6d908a4 to 42c1937 Compare November 15, 2024 14:17
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.

This feels a lot less magical than what was there before. One small comment.

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll Outdated Show resolved Hide resolved
@jketema
Copy link
Contributor

jketema commented Nov 15, 2024

DCA doesn't show any alert differences. How about MRVA?

@MathiasVP
Copy link
Contributor Author

Something odd going on when testing this locally. The DB/commit pair that seemed to work on DCA is now exploding locally 😰 I'm debugging this now!

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Nov 15, 2024

I've managed to figure out why DCA runs fine, but local runs hit a tuple explosion: The problem is that the IRGuardCondition::ensuresEq/4 is never actually evaluated in the security-and-quality suite. Instead queries use, GuardCondition::ensuresEq/4 (i.e., the AST interface version which for some reason re-implements the functionality so that it doesn't depend on the IRGuardCondition::ensuresEq/4 predicate) which doesn't have the same problem (because it's restricted to instruction for which getUnconvertedResultExpression has a result).

But because they're both in the same cached stage they get evaluated together when you don't specify --expect-discarded-cache (because the other predicate may be needed in some subsequent run that the evaluator isn't aware of). But when the flag is present the evaluator knows that the cache will be discarded and thus chooses not to evaluate it (at least, that's my mental model of --expect-discarded-cache).

Obviously we may want to call IRGuardCondition::ensuresEq/4 in the future, so the size of this predicate should be reduced so that we can evaluate it. I've got a fix that I'm testing now. Locally, this makes the IRGuardCondition::ensuresEq/4 feasible so I'm hopeful 🤞 But I'm running DCA on it now!

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