Skip to content

Conversation

@xiemaisi
Copy link

@xiemaisi xiemaisi commented Mar 25, 2019

This has turned into a fairly big PR, with three layers of commits (which I'd be happy to submit as separate PRs if desired):

  • The first group of commits replaces all current uses of tracked nodes (in the socket.io and HTTP libraries) with type tracking.
  • The second group of commits tweaks caching structure and refactors a few predicates to avoid unhelpful magic exposed by the first group of commits. These are intended to be behaviour-preserving.
  • The third group of commits extends type tracking with support for tracking through one level of properties, since otherwise we're losing results compared to tracked nodes. This entails a small change to the API, see the qldoc of TypeTracker and TypeBackTracker for details.

Performance is pretty awesome (internal link). The new results on NodeGoat look like (seeded) true positives; the regexp injection on semmle-express-test is, I think, a false positive that is unrelated to this PR (we assume every call to match with the right arity does regexp matching); and the remote-property injection results are harmless (the object into which the properties are injected has a null prototype).

@xiemaisi xiemaisi added the JS label Mar 25, 2019
@xiemaisi xiemaisi requested a review from a team as a code owner March 25, 2019 17:15
@xiemaisi xiemaisi force-pushed the js/more-type-tracking branch from 6ae98ae to c50067b Compare March 25, 2019 20:39
@asger-semmle asger-semmle self-assigned this Mar 26, 2019
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 overall

I was a bit sad to a see the style is different from the one I've been using and tried to advocate in a few places. Of course, that was just a style I made up and we haven't agreed upon as a team, but I think it's worth picking a one style and sticking with it. The differences between the two styles are currently:

  • What I have called t2 is here called s, prev, or next.
  • What I have called ref is here called flowsToSourceNode.
  • What I phrased as "with t summarising the data flow path" (although usually omitted) is here phrased as "with type tracking info stored in t".

Apart from the flowsToSourceNode change I wouldn't mind adopting the same style. If you're happy with these choices, could you update the qldoc and test cases in library-tests/TypeTracking to reflect this?

About caching: my intention was the StepSummary::{step, append, prepend} would be cached, and track and backtrack would be pragma[inline]. It was reassuring to see you arrived at something similar, although I think we can still get more by caching step. We'd need to find a way to cache both step and a reordered copy of step for use in backtrack, though that's something we can investigate later.

*
* Examples include arguments to the CommonJS `require` function or AMD dependency arguments.
*/
abstract class PathExprCandidate extends Expr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you confirm that the import graph is unchanged?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, good idea. It'll require rebuilding some snapshots, but seems worth doing.

@xiemaisi
Copy link
Author

Apart from the flowsToSourceNode change I wouldn't mind adopting the same style

Yeah, flowsToSourceNode is horrible; I'll change to ref.

I found myself preferring prev and next over t2 because it clarified in my mind which direction the tracking was happening in, and indeed I made a few mistakes that I caught that way. s is no good, though, I'll get rid of it.

I don't think there was any good reason for me changing the description of t, so I'll just revert to what you had earlier.

@xiemaisi xiemaisi added the WIP This is a work-in-progress, do not merge yet! label Mar 26, 2019
@xiemaisi
Copy link
Author

Naming suggestions addressed; running the import graph comparison now.

@xiemaisi xiemaisi force-pushed the js/more-type-tracking branch from 6141e35 to 3e16d16 Compare March 26, 2019 13:00
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.

Thanks for the update. Let's wait for the import graph check, but otherwise LGTM now.

@xiemaisi
Copy link
Author

No changes to PathExpr::resolve on our default projects. Is there anything else you'd like me to check?

@xiemaisi xiemaisi removed the WIP This is a work-in-progress, do not merge yet! label Mar 27, 2019
@asger-semmle
Copy link
Contributor

Good enough for me

@semmle-qlci semmle-qlci merged commit 4d4055a into github:master Mar 27, 2019
@xiemaisi xiemaisi deleted the js/more-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