Skip to content

C++: mass enable diff-informed data flow #19663

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

d10c
Copy link
Contributor

@d10c d10c commented Jun 3, 2025

An auto-generated patch that enables diff-informed data flow in the obvious cases.

Builds on #18342 and https://github.com/github/codeql-patch/pull/88

An auto-generated patch that enables diff-informed data flow in the obvious cases.

Builds on github#18342 and github/codeql-patch#88
@github-actions github-actions bot added the C++ label Jun 3, 2025
@d10c d10c marked this pull request as ready for review June 4, 2025 11:39
@Copilot Copilot AI review requested due to automatic review settings June 4, 2025 11:39
@d10c d10c requested a review from a team as a code owner June 4, 2025 11:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Enables diff-informed data flow analysis by injecting an observeDiffInformedIncrementalMode stub into multiple DataFlow and TaintTracking configurations.

  • Adds a stub predicate observeDiffInformedIncrementalMode() { any() } to enable diff-informed incremental analysis.
  • Applies across various security checks (CWE rules) and likely-bugs modules.
  • Prepares QL libraries for improved incremental performance on code changes.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-611/XXE.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-190/IntegerOverflowTainted.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll Added stub observeDiffInformedIncrementalMode
cpp/ql/src/Likely Bugs/Conversion/CastArrayPointerArithmetic.ql Added stub observeDiffInformedIncrementalMode
cpp/ql/lib/experimental/semmle/code/cpp/security/PrivateCleartextWrite.qll Added stub observeDiffInformedIncrementalMode
Comments suppressed due to low confidence (2)

cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql:34

  • [nitpick] Add a brief comment above this predicate explaining its role in enabling diff-informed incremental analysis to help future maintainers understand its purpose.
predicate observeDiffInformedIncrementalMode() { any() }

cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql:34

  • No tests were added to verify that diff-informed data flow actually takes effect. Consider adding regression tests for one or two representative modules to validate this new mode behaves as expected.
predicate observeDiffInformedIncrementalMode() { any() }

@d10c d10c marked this pull request as draft June 5, 2025 16:03
@d10c
Copy link
Contributor Author

d10c commented Jun 5, 2025

It turns out that some of the generated changes in the PRs were not correct, e.g. because they should have also generated a getASelected{Source,Sink}Location() override but didn't (see Chuan-kai's comment here). So for now I'm putting them back in Draft until I make sure (via the patch script) that we are correctly handling all 3 documented query patterns, starting with the simplest one (both source and sink are used as location sources). Thanks for the review and stay tuned for an update as to what has changed in the meantime!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant