Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Feb 7, 2020

We were missing flow from context to this in this case, needed for flagging a CVE:

let forIn = require('for-in')
forIn(src, callback, context)

function callback(value, key) {
  let val = this[key];
  ...
}

There's a bunch of other cases where this happened, previously modelled in the type inference using a couple of AnalyzedThisXXX classes.

I've tweaked PartialInvokeNode a bit so this can be modelled as a partial invocation, and changed the AnalyzedThisInXXX classes to instead be partial invokes, and their contribution to type inference generalized to work for any partial invoke.

So far I've evaluated the last commit against the first one, to confirm that nothing had happens from the refactoring of the AnalyzedThis classes (and that it doesn't blow up), but it still needs a comparison against master to see what the constant-factor overhead is.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Feb 7, 2020
@asgerf asgerf requested a review from a team as a code owner February 7, 2020 14:45
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

That is a nice generalisation.
I have one concern about the backwards compatibility of this change, see the inline comment.

I think we can get by without additional tests, but did you also mean to skip a change note about general type inference improvements?

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 21, 2020
@asgerf
Copy link
Contributor Author

asgerf commented Feb 21, 2020

Evaluation on smoke-tests looks reasonable (in addition to the larger evaluation; see PR description). I've looked into the cost on amphtml and it seems fine, it just has a lot of bound functions.

@asgerf
Copy link
Contributor Author

asgerf commented Feb 21, 2020

did you also mean to skip a change note about general type inference improvements?

I don't think the type inference changes will affect any queries, and it's kind of hard to explain in a simple change note what we've changed here. I think I'll incorporate it in a general change note about call graph improvements in #2855.

@semmle-qlci semmle-qlci merged commit e163d8d into github:master Feb 21, 2020
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