Skip to content

Conversation

@MathiasVP
Copy link
Contributor

The signatureMatches predicate only related the Function column to the i column (and not to the name or type columns) which meant that it returned waaaayyyy to many Functions.

Functionally the behavior was correct since the resulting Function was properly related to those entities in the caller (i.e, the interpretElement0 predicate). But it did mean we had a rather large blowup in the number of results for this predicate (which our tests also showed).

Thanks for highlithing this to me, @jketema!

Commit-by-commit review recommended.

Copilot AI review requested due to automatic review settings November 8, 2025 16:41
@MathiasVP MathiasVP requested a review from a team as a code owner November 8, 2025 16:41
@github-actions github-actions bot added the C++ label Nov 8, 2025
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 8, 2025
@MathiasVP MathiasVP force-pushed the fix-cp-in-external-flow branch from 6c9d5f4 to dfdc2a6 Compare November 8, 2025 16:46
Copy link
Contributor

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

This PR refactors the C++ dataflow external flow signature matching logic to properly utilize namespace information when identifying functions. Previously, namespace was noted as "not being used" in a comment, but this change integrates namespace checking throughout the signature matching process.

Key changes:

  • Introduces a getFunction helper predicate to centralize function identification logic based on namespace, type, and name
  • Extends signatureMatches predicate to include namespace as a parameter, enabling proper namespace-aware signature matching
  • Refactors interpretElement0 to use the new helper, improving code maintainability by reducing duplication

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll Adds getFunction helper predicate, updates signatureMatches to use namespace parameter, refactors interpretElement0 to leverage new helper, removes outdated comment about namespace not being used, adjusts classHasQualifiedName bindingset annotation
cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.ql Updates test predicate arity from /5 to /6 to match the new signatureMatches signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MathiasVP
Copy link
Contributor Author

Please disregard the "Pull Request Overview" comment by Copilot. It completely misses the point of this PR 😭

@jketema
Copy link
Contributor

jketema commented Nov 8, 2025

Note that the coding standards failure is my fault, please ignore. I know how to fix this.

@jketema
Copy link
Contributor

jketema commented Nov 8, 2025

There seems to be a slowdown in DCA.

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Nov 8, 2025

There seems to be a slowdown in DCA.

I'll take a look. Note that the majority of the slowdown is coming from the unstable macOS project. But worth double checking anyway!

Edit: Yeah, it looks like the code generated for the new version of signatureMatches isn't as good as the code on main. I'll need to look at that on Monday 😭

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