Skip to content

Conversation

@asger-semmle
Copy link
Contributor

The first commit from #760

@asger-semmle asger-semmle requested a review from a team as a code owner January 14, 2019 15:47
@ghost ghost added the JS label Jan 15, 2019
ghost
ghost previously approved these changes Jan 15, 2019
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.
I suppose we are still waiting for the performance evaluation?
The change note can be added in one of the other child PRs of #760.

@asger-semmle
Copy link
Contributor Author

The evaluation from the parent PR should still apply as it includes this commit.

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.

Broadly LGTM, minor suggestions.

or
exists(Function f, DataFlow::Node mid, DataFlow::Node base |
exists(Function f, DataFlow::Node mid |
// `f` stores its parameter `pred` in property `prop` of a value that it returns,

Choose a reason for hiding this comment

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

Comment needs to be generalised slightly since we're no longer requiring that the value is returned.

Choose a reason for hiding this comment

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

Do we need a concomitant change in TrackedNodes.qll?

/**
* Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
*/
predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) {

Choose a reason for hiding this comment

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

Can we make this 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.

I moved this as well as returnedPropWrite into FlowSteps.qll so they are private and can be shared with TrackedNode.qll.

Technically this is breaking since DataFlow::returnedPropWrite was public and not marked as internal. But it's kind of an obscure thing to try and use from the outside, so rather than deprecating it I think a change note should suffice in this case, WDYT?

Choose a reason for hiding this comment

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

Definitely; that predicate being public was an oversight.

exists(Function f | calls(succ, f) |
returnExpr(f, pred, _)
or
succ instanceof DataFlow::NewNode and

Choose a reason for hiding this comment

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

Again, perhaps worth generalising the doc comment to explain about this case.

@asger-semmle asger-semmle force-pushed the class-receiver-propagation branch from f8b4bbd to a1c7f32 Compare January 16, 2019 11:15
@asger-semmle
Copy link
Contributor Author

Rebased to resolve conflicts, PTAL overall.

@xiemaisi xiemaisi merged commit bca941d into github:master Jan 16, 2019
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.

2 participants