Skip to content

C++: Suggestions to #15343 #39

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

Conversation

MathiasVP
Copy link
Collaborator

@bdrodes these are my suggestions to github#15343.

Commit-by-commit review recommended - especially if you're not yet comfortable with parameterized modules!

  • The first commit does a straightforward refactoring of the interface of FlowFromFree: instead of passing two predicates as parameters to the module I wrap the "input predicates" into a module signature and change the FlowFromFree module to take such an "input module" as a parameter.
  • The next commit is really the meat of my suggestion: it adds a new predicate to the "input module" that represents additional requirements on the relationship between the source and the sink. In particular, for the github/codeql queries this can be the dominance/post-domination relationship, and for microsoft/codeql this defaults to any() meaning that no additional relationship is required.
  • The next commit makes cpp/use-after-free and cpp/double-free conform to this new interface.
  • The final commit accepts the test changes. This basically undoes the changes you saw in Generalization of FlowAfterFree github/codeql#15343.

The change I'm proposing here should mean that there are no performance changes compared to what's running currently in Code Scanning 🤞. But obviously we need to run DCA on this to confirm

@bdrodes
Copy link

bdrodes commented Jan 17, 2024

Interesting solution. I'm going to run with it. My only concern is that an 'any' predicate would be somehow inefficient, but I assume there are optimizations to avoid explosion with these?

@bdrodes bdrodes merged commit 39dafd6 into microsoft:38-cpp-generalize-use-after-free-libraries Jan 17, 2024
@MathiasVP
Copy link
Collaborator Author

Interesting solution. I'm going to run with it. My only concern is that an 'any' predicate would be somehow inefficient, but I assume there are optimizations to avoid explosion with these?

Note that the predicate is marked with a bindingset annotation (which means that the arguments are already bound). So there's no explosion. If we removed that annotation it would produce a cartesian product.

ropwareJB pushed a commit that referenced this pull request Jan 18, 2024
* C++: Change the interface of 'FlowAfterFree' so that the module it takes
a single module as a parameter.

* C++: Add another predicate to the module signature.

* C++: Convert the use-after-free and double-free libraries to use new interface.

* C++: Accept test changes.
bdrodes pushed a commit that referenced this pull request Jan 9, 2025
bdrodes pushed a commit that referenced this pull request Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants