Skip to content

Conversation

esbena
Copy link
Contributor

@esbena esbena commented Jan 12, 2021

This reintroduces the reverted js/resource-exhaustion - see original PR and its revert.

This is a bit of a sad PR. The first commit contains a query with flow labels, the second commit removes the use of the flow labels completely. The flow labels were used to distinguish between numbers and other types, since some sinks only works with numbers. I do not think the overhead of the flow labels is worth the potential results (which I expect to be very rare). The result is a faster query with fewer sinks.

I think the likelihood that the remaining sink setTimeout(SINK) will be hit in the wild is high enough that I want to merge the query in this restricted version instead of shelving the full query among the queries that are not run by default. We can add flow labels later to regain the remaining sinks.

Sample run: https://lgtm.com/query/2428900331474891062/

Can I get a proper dist-compare run? With the security suite or another RemoteFlow taint query for establishing the baseline?

@esbena esbena added the JS label Jan 12, 2021
@esbena esbena requested review from mchammer01 and a team as code owners January 12, 2021 12:49
@esbena
Copy link
Contributor Author

esbena commented Jan 12, 2021

I have removed the documentation label since the documentation has been reviewed in #3702 already.

@esbena
Copy link
Contributor Author

esbena commented Jan 13, 2021

(the check failure is just the usual one for missing qldoc on the Customization module pattern)

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jan 13, 2021
@erik-krogh
Copy link
Contributor

It all looks good to me (assuming that an evaluation looks ok).

@erik-krogh
Copy link
Contributor

I made an evaluation against a where none() edition of the query.

Definitely shows a slowdown.
I'm not sure if the slowdown is acceptable.

@esbena esbena force-pushed the js/reintroduce-resource-exhaustion branch from 65bfdd2 to b90dd89 Compare January 21, 2021 08:09
@esbena
Copy link
Contributor Author

esbena commented Jan 21, 2021

Thanks for checking. Lets shelve this in /experimental for now then (I have moved all files there).

@esbena esbena removed the request for review from mchammer01 January 21, 2021 09:21
@codeql-ci codeql-ci merged commit 30015ee into github:main Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish documentation JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants