Skip to content

C++: More TPs from cpp/using-expired-stack-address #8321

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

Merged

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Mar 3, 2022

This PR changes cpp/using-expired-stack-address so that the must-flow in the query follows flow into predicates. This should allow us to catch more true positive results (in particular, it fixes a missing result in our tests). I suggest reviewing the PR on a commit-by-commit basis.

  • 9df923a fixes the missing must-flow
  • bf10456 adds a path explanation to the query. This was a huge help when debugging the query results on LGTM, and I expect users will have the same opinion.

I've marked this as a draft while I run DCA. DCA looks good.

@MathiasVP MathiasVP added the C++ label Mar 3, 2022
@MathiasVP MathiasVP marked this pull request as ready for review March 4, 2022 08:37
@MathiasVP MathiasVP requested a review from a team as a code owner March 4, 2022 08:37
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Mar 4, 2022
Copy link
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.

Happy with the new results. Its definitely nice to have paths from this query as well (e.g. https://lgtm.com/query/4938718690567093881/). 👍

The QL is difficult to review though - feels like in an ideal world most of it would be in a separate library with a clear spec and tests - but I suppose there's only one place we'd be using it at present.

@MathiasVP
Copy link
Contributor Author

The QL is difficult to review though - feels like in an ideal world most of it would be in a separate library with a clear spec and tests - but I suppose there's only one place we'd be using it at present.

Indeed. There is some similarity between this new query, the cpp/return-stack-allocated-memory query, and the unsafe-use-of-this (i.e., they all use their own version of a must-flow relation). I'll create an issue that describes this in more detail. Hopefully, we can merge some if this code.

@MathiasVP MathiasVP merged commit 9a91e66 into github:main Mar 4, 2022
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