JS: Captured Nodes, type inference + a query #946

ghost · 2019-02-15T14:41:59Z

This PR introduce the concept of a "captured node", a captured node is a node whose that the dataflow library can reason about soundly. As a first example, I have introduced a CapturedSource for the object literals that never flow into a heap location or another site that we can not track with the dataflow library.

I believe the analysis for when a captured node escapes is sound (except for some very unlikely prototype fiddling, see the comments in the code), but I invite you to think hard if I have missed a case.

I have used the captured nodes for two separate things:

The main feature of this PR is an improvement for the type inference: we can now track types out of some method calls. We could also use captured nodes to improve the type inference for plain property reads, but then we will get a bunch of false positives to due lack of refinement nodes for properties (requires a separate CFG improvement).

The secondary feature of this PR is a new query that detects unused properties (similar to js/unused-variable). The false positives from this query ve been enourmously useful for identifying unsoundness in the characterization of a captured node.

The new alerts look good, and the performance overhead is negligible.

The evaluation of the Type inference improvement shows surprisinlyg few new results. Maybe a better propagation of information from the global type inference layer could help.

The evaluation of the js/unused-property query is surprisingly full of new results (10k+!), which is a great indicator for the potential of additional use of the captured node analysis. I have looked at a lot of the results, and I relatively feel confident in them now. The query is just a recommendation, so any remaining unsoundness will not be a disaster. (the timeout for the closure library is not related to this improvement, it only happens on azure)

asger-semmle

Could you run a benchmark with definitions.ql included so the cost of the new query is more obvious?

It's a shame the type inference doesn't give more results. Non-escaping object literals probably aren't all that common outside of compiled code.

asger-semmle · 2019-02-15T15:02:48Z

javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll

+      w = s.getAPropertyWrite() and
+      fun.flowsTo(w.getRhs()) and
+      (
+        not exists(w.getPropertyName())


Any dynamically stored property is assumed to be a call target? Please explain.

This is just standard soundness: we can not exclude any callees.

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

asger-semmle · 2019-02-15T15:15:40Z

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

+ * A source for which the flow is entirely captured by the dataflow library.
+ * All uses of the node is represented by `this.flowsTo(_)` and friends.
+ */
+class CapturedSource extends DataFlow::SourceNode {


Can we please call this something else? Capture usually has to do with closures and captured variables, where it means something completely different. The name CapturedSource seems to suggest it is a source node referenced by an inner function, like:

let obj = {}; return function() { return obj.x; };

I see. I agree that we should call it something different, but I am not satisfied with my other names. Do you have a suggestion?

For reference, some of my dismissed prefixes:

Local

this is my favorite, and we have LocalFunction already. I feel that LocalNode clashes with the concept of global variables though. LocallyUsedNode is a slightly longer alternative, with a clearer relation to its semantic.

Trackable

clashes with TrackedNode

Restricted

NonEscaping

Sound

Yeah, veto on "captured" from me as well. Since it only handles object literals, would "LocalObject" perhaps be an option?

javascript/ql/src/Declarations/UnusedProperty.ql

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

javascript/ql/src/Declarations/UnusedProperty.ql

ghost · 2019-02-21T20:37:16Z

Clearer evaluation with a comparison table, it is remarkably fast.

xiemaisi

I'm not quite through with this yet, but here are a few comments to consider. Looking very nice overall!

javascript/ql/src/Declarations/UnusedProperty.ql

javascript/ql/src/Declarations/UnusedVariable.qll

javascript/ql/src/Expressions/ExprHasNoEffect.qll

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

xiemaisi · 2019-02-25T09:59:27Z

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

+ * A source for which the flow is entirely captured by the dataflow library.
+ * All uses of the node is represented by `this.flowsTo(_)` and friends.
+ */
+class CapturedSource extends DataFlow::SourceNode {


Yeah, veto on "captured" from me as well. Since it only handles object literals, would "LocalObject" perhaps be an option?

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

xiemaisi

A few more thoughts.

xiemaisi · 2019-02-25T12:16:07Z

javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll

+      s.hasOwnProperty(name) and
+      hasDefiniteReceiver(this, s) and
+      w = s.getAPropertyWrite() and
+      fun.flowsTo(w.getRhs()) and


I assume we guard somewhere else against the case that the RHS is a function that we cannot track?

Yes, a LocalObject object can not have an unknown method, because that method could expose the object through this.

javascript/ql/src/semmle/javascript/dataflow/internal/InterProceduralTypeInference.qll

javascript/ql/test/library-tests/CapturedNodes/method-calls.js

ghost · 2019-02-25T15:12:15Z

All comments addressed

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll

xiemaisi · 2019-02-26T08:47:21Z

LGTM; handing over to @Semmle/doc for query help review.

xiemaisi · 2019-03-01T08:21:59Z

Reminder to @Semmle/doc: this is a 1.20 PR that still needs a query help review.

mchammer01 · 2019-03-01T09:44:05Z

Yes sorry @xiemaisi you used to mention me but I don't always pick up mentions of @semmle/doc, especially in busy times like this. Will try to do the review for this shortly.

mchammer01

@esben-semmle and @xiemaisi - apologies for the late review, this passed me by.
This LGTM.
I've made a very minor comment that you can ignore if you want. I leave it up to you (I approved this PR as the issue I raised is minor).

mchammer01 · 2019-03-01T09:55:23Z

javascript/ql/src/Declarations/UnusedProperty.qhelp

+	<example>
+
+		<p>
+			In this code, the function <code>f</code> initializes a property <code>prop_a</code> with a


Suggesting adding a comma between but later on and this property, to aid comprehension

ghost · 2019-03-01T10:46:42Z

I think we should merge this now then, to get it into 1.20 as easily as possible.

I noticed that the name 'captured' still is used in variables and tests, so I will fix that while adding mc's suggested comma.

ghost added the JS label Feb 15, 2019

ghost self-requested a review as a code owner February 15, 2019 14:41

asger-semmle reviewed Feb 15, 2019

View reviewed changes

Esben Sparre Andreasen added 8 commits February 21, 2019 21:44

JS: fixup changenote for js/unbound-event-handler-receiver

bfbf686

JS: introduce CapturedSource

0cf2eae

JS: add query js/unused-property

91dccc3

JS: avoid double reporting dead code with js/unused-variable

8af501d

JS: change notes for js/unused-property and js/unused-variable

c84d898

JS: add type inference for the return value of captured method calls

bdd8691

JS: add PropWrite tests for parameter field initializers

6766716

JS: add missing flowstep for unused parameter field initializers

6c1b29e

xiemaisi reviewed Feb 25, 2019

View reviewed changes

Esben Sparre Andreasen added 7 commits February 25, 2019 15:19

JS: address review comments

047b69a

JS: analyze assignments in with correctly

0d94fe3

JS: add documentation to test case

1150f4c

JS: format test case (update expected output)

65fb142

JS: rename CapturedSource -> LocalObject

6636798

JS: rename CapturedSource -> LocalObject (files)

4dc147d

JS: update docstring

ab1b1c1

xiemaisi reviewed Feb 26, 2019

View reviewed changes

javascript/ql/src/semmle/javascript/dataflow/CapturedNodes.qll Outdated Show resolved Hide resolved

xiemaisi assigned mchammer01 Feb 26, 2019

JS: address review comment

9511bdf

xiemaisi added this to the 1.20 milestone Feb 28, 2019

mchammer01 approved these changes Mar 1, 2019

View reviewed changes

xiemaisi merged commit 83e0f3b into github:master Mar 1, 2019

This was referenced Mar 1, 2019

JS: replace stale mentions of 'captured' with 'local' #1019

Closed

JS: cherry pick #946 to rc/1.20 #1028

Closed

JS: Captured Nodes, type inference + a query #946

JS: Captured Nodes, type inference + a query #946

Uh oh!

Conversation

ghost commented Feb 15, 2019

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Feb 21, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Feb 21, 2019

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Feb 25, 2019

Uh oh!

Uh oh!

xiemaisi commented Feb 26, 2019

Uh oh!

xiemaisi commented Mar 1, 2019

Uh oh!

mchammer01 commented Mar 1, 2019

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost Feb 21, 2019 •

edited by ghost

Loading