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++: Resolve typedefs when matching MaD parameters #18386

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

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jan 2, 2025

The various implementations of the STL differ on the exact name of the types of some of the parameters. For example, some implementations specify std::vector::insert as:

template<typename T /*, ... */>
class vector {
  /* ... */
  iterator insert(const_iterator, const T&);
}

while others do this instead:

template<typename T /*, ... */>
class vector {
  /* ... */
  iterator insert(const_iterator, const value_type&);
}

these are completely identical since containers are required to declare value_type as a synonym for T. However, until this PR we would match the parameter type name against whatever type name is specified. With the current MaD models, this meant we only matched against the implementations that picked const T&, but not the ones that picked const value_type&.

This PR fixes this by allowing both the unresolved and the typedef-resolved type name. This ensures that we can continue to use the typedef'ed name when it's convenient (for example, when a parameter must have type size_type which is platform dependent).

Our Type class actually comes with a perfectly reasonable predicate for resolving typedefs while retaining whatever specifiers are attached to the type:

* Gets this type with any typedefs resolved. For example, given
* `typedef C T`, this would resolve `const T&` to `const C&`.
* Note that this will only work if the resolved type actually appears
* on its own elsewhere in the program.
*/
Type resolveTypedefs() { result = this }
However, as noted by the QLDoc this predicate only has a result when the resulting type (i.e., the typedef-resolved type with the same specifiers) exists in the database. So to get around this we have to create our own predicate for resolving typedefs while retaining the specifiers.

Commit-by-commit review recommended:

  • 5ccc12c adds a failing testcase
  • 3d3feb6 resolves typedefs
  • 682dd42 accepts testcases
  • 9672af3 adds a nice small performance optimization I spotted while working on this PR

@github-actions github-actions bot added the C++ label Jan 2, 2025
@MathiasVP MathiasVP marked this pull request as ready for review January 3, 2025 17:56
@Copilot Copilot bot review requested due to automatic review settings January 3, 2025 17:56
@MathiasVP MathiasVP requested a review from a team as a code owner January 3, 2025 17:56

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (6)
  • cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll: Language not supported
  • cpp/ql/test/library-tests/dataflow/models-as-data/FlowSummaryNode.expected: Language not supported
  • cpp/ql/test/library-tests/dataflow/models-as-data/SummaryCall.expected: Language not supported
  • cpp/ql/test/library-tests/dataflow/models-as-data/testModels.qll: Language not supported
  • cpp/ql/test/library-tests/dataflow/models-as-data/tests.cpp: Language not supported
  • cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected: Language not supported

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

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

Successfully merging this pull request may close these issues.

1 participant