Skip to content

C++: Add more MaD models for ATL string classes#18261

Merged
MathiasVP merged 16 commits intogithub:mainfrom
MathiasVP:add-more-atl-string-models
Jan 2, 2025
Merged

C++: Add more MaD models for ATL string classes#18261
MathiasVP merged 16 commits intogithub:mainfrom
MathiasVP:add-more-atl-string-models

Conversation

@MathiasVP
Copy link
Copy Markdown
Contributor

Follow-up to #18136

Note that I also fixed another MaD bug: we didn't handle varargs. This has been fixed in c5bb907

@MathiasVP MathiasVP requested a review from a team as a code owner December 10, 2024 18:30
@github-actions github-actions Bot added the C++ label Dec 10, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Dec 10, 2024
@MathiasVP MathiasVP requested a review from geoffw0 December 10, 2024 20:30
dbartol
dbartol previously approved these changes Dec 20, 2024
Copy link
Copy Markdown

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

Looks good, at least from what I can remember from 25 years ago. I noted one trivial missing model, not blocking.

- ["", "CSimpleStringT", True, "operator+=", "(wchar_t)", "", "Argument[-1]", "ReturnValue[*]", "taint", "manual"]
- ["", "CSimpleStringT", True, "operator+=", "(wchar_t)", "", "Argument[0]", "ReturnValue[*]", "taint", "manual"]
- ["", "CSimpleStringT", True, "operator+=", "(wchar_t)", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
- ["", "CSimpleStringT", True, "operator=", "", "", "Argument[*0]", "Argument[-1]", "value", "manual"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does operator= return a CSimpleStringT&? If so, Argument[*0] should propagate to ReturnValue[*] as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed it does: https://learn.microsoft.com/en-us/cpp/atl-mfc-shared/reference/csimplestringt-class?view=msvc-170#operator_eq. Turns out I actually forgot that flow in a bunch of other models. I've included all of those in e777377. Thanks!

Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Minor corrections / comments, otherwise LGTM. Thanks for addressing the missing operator= return value models in addition to the main purpose of this PR.

Comment thread cpp/ql/lib/ext/CSimpleStringT.model.yml Outdated
Comment thread cpp/ql/lib/ext/CSimpleStringT.model.yml Outdated
Comment thread cpp/ql/lib/ext/CStringT.model.yml Outdated
Comment thread cpp/ql/test/library-tests/dataflow/taint-tests/atl.cpp Outdated
Comment thread cpp/ql/test/library-tests/dataflow/taint-tests/atl.cpp
Comment thread cpp/ql/lib/ext/CStringT.model.yml Outdated
Comment thread cpp/ql/lib/ext/CStringT.model.yml
Comment thread cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll
@MathiasVP
Copy link
Copy Markdown
Contributor Author

Minor corrections / comments, otherwise LGTM. Thanks for addressing the missing operator= return value models in addition to the main purpose of this PR.

Yet another round of 🦅-eyed reviewing, Geoffry! Thanks a lot. I've responded to all the comments now 🙇

Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM.

@MathiasVP MathiasVP merged commit f23e56b into github:main Jan 2, 2025
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.

3 participants