Skip to content

ImproperCodeSanitization is much slower than other queries #11679

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

Open
jakebailey opened this issue Dec 13, 2022 · 3 comments
Open

ImproperCodeSanitization is much slower than other queries #11679

jakebailey opened this issue Dec 13, 2022 · 3 comments
Labels
question Further information is requested

Comments

@jakebailey
Copy link

Description of the issue

This is a follow-up to #10937.

In microsoft/TypeScript#51878, I am removing the exclusions for all of the slow rules mentioned in #10937, and the build now completes without timing out the builder.

But, it's running quite a bit slower than expected; reenabling all of these rules doubles the code scanning time. It appears as though all of this extra time comes from one rule: https://github.com/microsoft/TypeScript/actions/runs/3688169461/jobs/6242652775#step:5:1809

[88/88 eval 10m18s] Evaluation done; writing results to codeql/javascript-queries/Security/CWE-094/ImproperCodeSanitization.bqrs.

All of the other rules complete in about 3 minutes or less, so this one seems like an outlier to be taking 3x more time than any other rule. It's not a blocker or anything, but I figured I'd report it in case there were some more perf wins to be had.

@erik-krogh
Copy link
Contributor

I can replicate the slow query, but I don't think there's much I can do about it.

I think getting the code to run faster would require a more complicated dataflow analysis that work in more "stages" than the 2 stages we currently use in the JavaScript analysis.
We actually have that, it's called the shared dataflow library, and it's used by all other languages than JavaScript.

We have some loose plans that we will some day move JavaScript to use the shared dataflow library, and that will hopefully fix performance issues like these automatically 🤞

In the meantime, if you want the CI job to run faster I can recommend using a larger worker.
I think changing ubuntu-latest to ubuntu-latest-xl should speed up things by at least a few minutes.

@jakebailey
Copy link
Author

All good, I just noticed the slowdown and figured I'd report it.

I didn't realize GHA was offering bigger instances either!

@erik-krogh
Copy link
Contributor

All good, I just noticed the slowdown and figured I'd report it.

And we're glad you did, sometimes there is something obvious that we can fix.
So keep them coming 👍

I didn't realize GHA was offering bigger instances either!

I'm not sure about the details, I just know that we use ubuntu-latest-xl a lot in the github/ org.
But those xl workers don't seem to be documented, so I don't think they'll work for you. Sorry if I got your hopes up.

But I did find this thing: https://github.blog/2022-09-01-github-actions-introducing-the-new-larger-github-hosted-runners-beta/
Which seems to configureable bigger instances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants