Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Aug 27, 2025

This is (unsurprisingly) quite an important model to Microsoft.
I've tested it on a random Microsoft database I had locally, and the number of summary local flow steps goes up by ~11% 🎉

@MathiasVP MathiasVP requested a review from a team as a code owner August 27, 2025 12:32
@Copilot Copilot AI review requested due to automatic review settings August 27, 2025 12:32
@github-actions github-actions bot added the C++ label Aug 27, 2025
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

This PR adds MaD (Modeling as Data) models for Microsoft's ComPtr template class, which is a smart pointer used extensively in Windows development. The models enable proper data flow tracking through ComPtr operations, resulting in an ~11% increase in summary local flow steps on Microsoft codebases.

  • Adds comprehensive MaD models for ComPtr constructors, methods, and operations
  • Includes test coverage with taint flow scenarios for various ComPtr operations
  • Updates expected test results to reflect the new data flow capabilities

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
cpp/ql/lib/ext/ComPtr.model.yml Defines MaD models for ComPtr constructors, methods like Get/Detach/CopyTo, and data flow patterns
cpp/ql/test/library-tests/dataflow/taint-tests/atl.cpp Adds comprehensive test cases for ComPtr taint tracking scenarios
cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected Updates expected results for local taint flow tests
cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected Updates expected MaD signature matching results
cpp/ql/test/library-tests/dataflow/external-models/validatemodels.expected Updates model validation results

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Aug 27, 2025
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

One small comment. Otherwise this LGTM, assuming DCA is happy.

int x = source<int>();
Microsoft::WRL::ComPtr<int> p1(new int(x));
int *raw = nullptr;
p1.CopyTo(&raw);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is using the template version right? I seem to be missing a test for the non-template, 1 argument version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes you're right! We were in fact missing a model for that one (see 8c07a3e). I've fixed this in bebfe03

@MathiasVP
Copy link
Contributor Author

DCA was uneventful (as expected since we dont have a lot of these things covered in DCA)

@MathiasVP MathiasVP merged commit bb08611 into github:main Aug 27, 2025
14 checks passed
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