Skip to content

Conversation

@xiemaisi
Copy link

These steps need to check that the type hasn't been tracked into a property.

Before I start testing this, I'd like some feedback on the API. Checking for the property name explicitly is awkward and easy to forget and should probably be encapsulated in an auxiliary predicate, but I'm struggling to come up with an intuitive name and would be grateful for suggestions. Or maybe there is another way to ensure this?

Max Schaefer added 2 commits March 28, 2019 10:12
These steps need to check that the type hasn't been tracked into a property.
@xiemaisi xiemaisi added the JS label Mar 28, 2019
@asger-semmle
Copy link
Contributor

We could add a self-returning predicate like use():

class TypeTracker {
  TypeTracker use() {
    prop = "" and result = this
  }
}

This only happens in cases where we're passing t into the recursive call, and that's where I'd advocate for its use:

exists(string name |
  result = myType(t.use()).getAMethodCall(name)
|
  name = "foo" or name = "bar"
)

@xiemaisi
Copy link
Author

I like that idea a lot! A more descriptive name than use would be nice; I'll think about it.

@xiemaisi xiemaisi added the WIP This is a work-in-progress, do not merge yet! label Mar 28, 2019
@xiemaisi xiemaisi marked this pull request as ready for review March 28, 2019 17:12
@xiemaisi xiemaisi requested a review from a team as a code owner March 28, 2019 17:12
@asger-semmle
Copy link
Contributor

I doubt I'd use it very often, though. I usually just do t.start() and use the "end" version of my predicate to throw away the calling context when a sufficiently specific use is found (as per the firebase.database example from the Firebase PR description).

@xiemaisi
Copy link
Author

I usually just do t.start() and use the "end" version of my predicate to throw away the calling context when a sufficiently specific use is found

Yes, I'm sure that is in general the more common pattern. However, when tracking the precise origin of flow (and not just the type), it seems dangerous to throw away calling contexts, so that's why I'm doing it this way.

@xiemaisi
Copy link
Author

I've encapsulated the t2.getProp() = "" and t = t2 pattern in a new predicate t = t2.continue().

Evaluation on socket.io-relevant slugs looks fine. The evaluation was before the last commit, but the DIL changes from that commit look harmless; I can do another sanity check if desired.

Copy link
Contributor

@asger-semmle asger-semmle left a comment

Choose a reason for hiding this comment

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

LGTM

@xiemaisi xiemaisi added JS and removed JS WIP This is a work-in-progress, do not merge yet! labels Apr 1, 2019
@semmle-qlci semmle-qlci merged commit a7d9a50 into github:master Apr 1, 2019
@xiemaisi xiemaisi deleted the js/fix-socket-io-type-tracking branch April 2, 2019 07:26
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