C++: Diff-informed queries: phase 3 (non-trivial locations) - #20073
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR enables diff-informed mode on C++ security queries that select locations other than dataflow source or sink. The changes add location override predicates to allow these queries to work with diff-informed analysis, which is part of the final phase of mass-enabling diff-informed queries across all languages.
Key changes include:
- Addition of observeDiffInformedIncrementalMode() predicates to enable diff-informed mode
- Implementation of getASelectedSourceLocation() and getASelectedSinkLocation() predicates to specify relevant locations
- Creation of test files for CWE-020 (External API) and CWE-313 (Cleartext SQLite Database) queries
Reviewed Changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Multiple security query files | Added diff-informed mode configuration with location override predicates |
| CleartextSqliteDatabase test files | Added comprehensive test case with SQLite security vulnerabilities |
| ExternalAPI test files | Added test infrastructure for external API queries |
| ExternalAPIsSpecific.qll files | Disabled diff-informed mode for library code used by multiple queries |
Sorry, something went wrong.
michaelnebel
left a comment
There was a problem hiding this comment.
The QL changes LGTM, but I am not able to help with writing tests for C++. Please reach out in the codeql-c slack channel.
Note that the DCA run didn't include the experimental queries (they are not included in nightly.qls).
Sorry, something went wrong.
|
I have split off the CWE-020 query and its test into a separate PR in order not to block the rest of the commits. I will start a security-experimental DCA experiment. |
Sorry, something went wrong.
michaelnebel
left a comment
There was a problem hiding this comment.
QL changes LGTM!
Sorry, something went wrong.
|
The nightly/security-experimental DCA experiment has timeouts across the board, so it's not giving us much useful information. However the performance impact on the non-empty-diff projects that didn't time out is negligible (if not noise), so I'd say it's good enough to merge. |
Sorry, something went wrong.
This PR enables diff-informed mode on queries that select a location other than dataflow source or sink. This entails adding a non-trivial location override that returns the locations that are actually selected.
Prior work includes PRs like #19663, #19759, and #19817. This PR uses the same patch script as those PRs to find candidate queries to convert to diff-enabled. This is the final step in mass-enabling diff-informed queries on all the languages.
Commit-by-commit reviewing is recommended.
Potentially tricky cases: