Skip to content

Swift: Modernize injection queries #11979

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 6 commits into from
Jan 31, 2023
Merged

Swift: Modernize injection queries #11979

merged 6 commits into from
Jan 31, 2023

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jan 24, 2023

Modernize the injection queries swift/unsafe-webview-fetch, swift/sql-injection and swift/unsafe-js-eval. The queries are now split into a minimal Abc.ql file mostly containing metadata, an AbcQuery.qll library containing the query logic (dataflow configuration in these cases) and an AbcExtensions.qll containing sources/sinks/etc that might be reusable. This layout makes it easier to extend, test and customize existing queries, and to share sources/sinks/etc between them.

This change should not affect query results.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Jan 24, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner January 24, 2023 17:37
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jan 24, 2023

I'm going to fix the failing check and add a few more commits to this PR tomorrow...

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jan 25, 2023

(checks are now passing, ready for review)

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! Do we want to run a DCA for this?

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jan 30, 2023

Do we want to run a DCA for this?

My initial thought was probably not, because this PR is not meant to be behaviour changing. But on the other hand there are several changes that could perhaps affect performance (also, someone saying we "probably" don't need a DCA run might be a good heuristic for when we do in fact need to do a DCA run)...

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jan 31, 2023

DCA: a little bit more than the usual level of wobble, but it looks fine to me:

  • analysis / query execution took a fair bit longer than normal on HelioMesquita__Swiftmazing and to a lesser extent DanielZSY__RxCommonKit, but this is compensated for by running faster on calleluks__Tofu and AndrewBennet__ReadingList. No signs of a pattern here.
  • we saw some stage timings wobbles as well, none of them are on the queries this PR touches.

@geoffw0 geoffw0 merged commit ee442e4 into github:main Jan 31, 2023
@geoffw0 geoffw0 deleted the modern1 branch March 31, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants