Skip to content

C++: Clean up the XXE query QL. #9176

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
merged 9 commits into from
May 17, 2022
Merged

C++: Clean up the XXE query QL. #9176

merged 9 commits into from
May 17, 2022

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented May 16, 2022

This is the promised follow-up PR to clean up the somewhat messy XXE query code, with a unifying XmlLibrary class defining sources and sinks. Then split the QL for the query into four files:

  • XXE.ql, containing the query itself, now a straightforward dataflow query.
  • XXE.qll, containing the abstract classes / models used by the query.
  • Xerces.qll, containing the implementation details for all four supported Xerces interfaces.
  • libxml2.qll, containing the implementation details for the libxml2 library.

This makes the code considerably easier to read, navigate, and potentially extend. There are a few limitations as things stand though:

  • I have not attempted to further split Xerces.qll into the individual interfaces. Its easily the largest of the four files and it would be logical to break it up, but there's a fair bit of crossover between the interfaces that might make it tricky to do so.
  • I have not merged the existing XXEFlowStateTransformer class into XmlLibrary. Again it would be logical, but the crossover between the Xerces interfaces that might make this more difficult than it seems.
  • I have not attempted to make the XML.qll library general purpose. It resides in the query directory. I can imagine a future where we have an XML library in models, and such a thing could grow from what we have here, but right now what it offers is quite specialized to the needs of the XXE query in particular.

There should be no changes to query results from this PR.

I'll check performance locally and with another DCA run.

@geoffw0 geoffw0 added C++ no-change-note-required This PR does not need a change note labels May 16, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner May 16, 2022 12:05
@geoffw0
Copy link
Contributor Author

geoffw0 commented May 16, 2022

Local performance is about the same as before.

@geoffw0
Copy link
Contributor Author

geoffw0 commented May 17, 2022

DCA looks fine as well.

MathiasVP
MathiasVP previously approved these changes May 17, 2022
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.

LGTM! I've got one single comment that you can choose to address or disagree with :)

@geoffw0 geoffw0 merged commit 629e90f into github:main May 17, 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