Skip to content

C++: Diff-informed queries: phase 3 (non-trivial locations) - #20073

Merged
d10c merged 24 commits into
github:mainfrom
d10c:d10c/diff-informed-phase-3-cpp
Aug 18, 2025
Merged

C++: Diff-informed queries: phase 3 (non-trivial locations)#20073
d10c merged 24 commits into
github:mainfrom
d10c:d10c/diff-informed-phase-3-cpp

Conversation

@d10c

@d10cd10c commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

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.

  • I have split the commits that add/modify tests from the ones that enable/disable diff-informed queries.
  • If the commit modifies a .qll file, in the commit message I've included links to the queries that depend on that .qll for easier reviewing.
  • Feel free to delegate parts of the review to others who may be more specialized in certain languages.

Potentially tricky cases:

@d10cd10c added the no-change-note-required This PR does not need a change note label Jul 17, 2025
@d10c
d10c requested a review from michaelnebelJuly 17, 2025 13:40
@d10c
d10c marked this pull request as ready for review July 17, 2025 13:40
CopilotAI review requested due to automatic review settings July 17, 2025 13:40
@d10c
d10c requested a review from a team as a code ownerJuly 17, 2025 13:40

CopilotAI left a comment

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.

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.

FileDescription
Multiple security query filesAdded diff-informed mode configuration with location override predicates
CleartextSqliteDatabase test filesAdded comprehensive test case with SQLite security vulnerabilities
ExternalAPI test filesAdded test infrastructure for external API queries
ExternalAPIsSpecific.qll filesDisabled diff-informed mode for library code used by multiple queries

@michaelnebelmichaelnebel left a comment

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.

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).

@d10c
d10cforce-pushed the d10c/diff-informed-phase-3-cpp branch from 46546b2 to 8978820CompareAugust 15, 2025 10:01
@d10c

d10c commented Aug 15, 2025

Copy link
Copy Markdown
ContributorAuthor

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.

@michaelnebelmichaelnebel left a comment

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.

QL changes LGTM!

@d10c

d10c commented Aug 18, 2025

Copy link
Copy Markdown
ContributorAuthor

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.

@d10c
d10c merged commit f1b5564 into github:mainAug 18, 2025
15 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++no-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@d10c@michaelnebel