Skip to content

JavaScript: add flow steps through partial function application#162

Merged
semmle-qlci merged 5 commits intogithub:masterfrom
asger-semmle:partial-calls
Oct 2, 2018
Merged

JavaScript: add flow steps through partial function application#162
semmle-qlci merged 5 commits intogithub:masterfrom
asger-semmle:partial-calls

Conversation

@asger-semmle
Copy link
Contributor

Adds flow steps from the arguments of Function.prototype.bind into the parameters of the target function, and similar for _.partial and friends.

For example:

function foo(x, y) {
  sink(x);
}
array.forEach(foo.bind(null, taint()));

Pending evaluation as we're a bit low on worker machines at the moment.

@asger-semmle asger-semmle requested a review from a team as a code owner September 5, 2018 15:59
@ghost ghost self-assigned this Sep 6, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Bold addition. How did the evaluation turn out?

I have a few comments and suggestions.

override predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index) {
callback = getArgument(0) and
exists (DataFlow::ArrayLiteralNode array |
array = getArgument(1) and
Copy link

Choose a reason for hiding this comment

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

Did you deliberately avoid array.flowsTo(getArgument(1))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems safe enough, I'll change it and give it a go.

*/
predicate partiallyCalls(DataFlow::AdditionalPartialInvokeNode invk, DataFlow::Node callback, Function f) {
invk.isPartialArgument(callback, _, _) and
exists (AbstractFunction callee | callee = callback.analyze().getAValue() |
Copy link

Choose a reason for hiding this comment

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

You can save the pseudo-cast by declaring callback with the type AnalyzedNode

*
* This only holds for explicitly modeled partial calls.
*/
predicate partiallyCalls(DataFlow::AdditionalPartialInvokeNode invk, DataFlow::Node callback, Function f) {
Copy link

Choose a reason for hiding this comment

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

This can be private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire library is internal and privately imported anyway, but sure.

@asger-semmle
Copy link
Contributor Author

Evaluation on big-apps. No new results and a bit of a slow-down.

I'll investigate the slow-downs while we wait for @max to get back.

@ghost ghost added JS WIP This is a work-in-progress, do not merge yet! labels Sep 9, 2018
ghost
ghost previously approved these changes Sep 9, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM, except for performance, I have approved and tagged with WIP.

xiemaisi
xiemaisi previously approved these changes Sep 24, 2018
Copy link

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

LGTM.

Optionally, I would support pulling out the logic for preventing cross-file global calls shared by calls and partiallyCalls into an auxiliary predicate (with an appropriate bindingset to enforce inlining if necessary).

@asger-semmle
Copy link
Contributor Author

Optionally, I would support pulling out the logic for preventing cross-file global calls shared by calls and partiallyCalls into an auxiliary predicate (with an appropriate bindingset to enforce inlining if necessary).

Makes sense. I'll get that in after working out the perf differences.

@asger-semmle asger-semmle dismissed stale reviews from xiemaisi and ghost via d919aa6 September 25, 2018 09:01
invk.isPartialArgument(callback, _, _) and
exists (AbstractFunction callee | callee = callback.getAValue() |
if invk.isIndefinite("global") then
if callback.getAValue().isIndefinite("global") then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optionally, I would support pulling out the logic for preventing cross-file global calls shared by calls and partiallyCalls into an auxiliary predicate (with an appropriate bindingset to enforce inlining if necessary).

After this bugfix the two aren't easy to unify anymore. invk.isIndefinite relies on the cached predicate getACalleeValue which we can't use here. Rewriting it turned out to be very costly, so I'd rather leave it like this.

@asger-semmle
Copy link
Contributor Author

The slowdown was caused by callInputStep being optimized differently, and me trying to cache AdditionalPartialInvokeNode which was a bad idea.

I've run Xss on big-apps and performance now looks better.

@asger-semmle asger-semmle removed the WIP This is a work-in-progress, do not merge yet! label Oct 1, 2018
@asger-semmle
Copy link
Contributor Author

@max are you happy with the perf evaluation or should I start a larger run?

@xiemaisi
Copy link

xiemaisi commented Oct 2, 2018

No, I think that's fine. @esben-semmle, what's your take?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This still LGTM.

@semmle-qlci semmle-qlci merged commit b35f450 into github:master Oct 2, 2018
@kamarcum kamarcum unassigned ghost Apr 28, 2020
aibaars added a commit that referenced this pull request Oct 14, 2021
Basic implementation of module resolution
smowton pushed a commit to smowton/codeql that referenced this pull request Jan 17, 2022
Java/Kotlin: Support mixed-language tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants