Skip to content

Generalization of FlowAfterFree #15343

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

bdrodes
Copy link
Contributor

@bdrodes bdrodes commented Jan 16, 2024

Updating FlowAfterFree to not enforce dominance of source/sink, now enforced by the instantiations of this module.

…eFree and UseAfterFree queries now enforce dominance.
@bdrodes bdrodes requested a review from a team as a code owner January 16, 2024 18:17
@github-actions github-actions bot added the C++ label Jan 16, 2024
@bdrodes
Copy link
Contributor Author

bdrodes commented Jan 16, 2024

@MathiasVP , I have some concerns in how I'm doing dominance checks, in that they are not optimized with any special annotations. I wonder if at scale this means dominance would be computed universally before joined with source/sinks.

@MathiasVP
Copy link
Contributor

MathiasVP commented Jan 16, 2024

@MathiasVP , I have some concerns in how I'm doing dominance checks, in that they are not optimized with any special annotations. I wonder if at scale this means dominance would be computed universally before joined with source/sinks.

Yes, I don't the approach you have here is how we should do it. With this approach we're:

  1. Probably materializing both sinkStrictlyPostDominatesSource and sourceStrictlyDominatesSink (unless the compiler inlines them) - and they're both pretty large predicates.
  2. Generating a lot of source/sink pairs before restricting it to the sinks that dominate the source. This is actually what we're seeing with the changes to the .expected file: dataflow is generating more tuples in the edges and nodes relations, which we're then excluding in the select clause with the domination/post-domination condition.

If you don't mind, I'll push a commit with some of the changes I think is necessary?

@MathiasVP
Copy link
Contributor

I've opened microsoft#39 for you to take a look at.

* 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
Copy link
Contributor Author

bdrodes commented Jan 17, 2024

I've opened microsoft#39 for you to take a look at.

I've accepted this. Let's role with it!

ropwareJB pushed a commit to microsoft/codeql 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.
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jan 18, 2024
@bdrodes
Copy link
Contributor Author

bdrodes commented Jan 18, 2024

Note. I added missing deallocation functions as part of this PR just now, but for some reason it is showing the entire file changed rather than a few of the definitions for deallocation were added to.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

The changes LGTM! Now we just need a happy DCA run and we can merge this 😄

@MathiasVP
Copy link
Contributor

I think I've fixed the CI errors here: microsoft#40. Basically, CI checks that everything "reachable" from a query file has QLDocs added to it. The solution to that CI check failing is always to either:

  • Add more QLDoc to public stuff, or
  • Make the accidental public stuff private

And in microsoft#40 I've done a bit of both.

@MathiasVP MathiasVP merged commit 145b5a3 into github:main Jan 23, 2024
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