Merge pull request #900 from esben-semmle/js/defuse-default

Approved by xiemaisi
2026-05-03 04:39:29 +02:00 · 2019-02-08 11:28:32 +00:00
parent a48594ad8e 5ad83360be
commit 7e298cfbbe
17 changed files with 146 additions and 11 deletions
--- a/javascript/ql/src/Declarations/DeadStoreOfLocal.ql
+++ b/javascript/ql/src/Declarations/DeadStoreOfLocal.ql
@@ -19,7 +19,7 @@ import DeadStore
 */
 predicate deadStoreOfLocal(VarDef vd, PurelyLocalVariable v) {
  v = vd.getAVariable() and
-  exists(vd.getSource()) and
+  (exists(vd.getSource()) or exists(vd.getDestructuringSource())) and
  // the definition is not in dead code
  exists(ReachableBasicBlock rbb | vd = rbb.getANode()) and
  // but it has no associated SSA definition, that is, it is dead
--- a/javascript/ql/src/semmle/javascript/DefUse.qll
+++ b/javascript/ql/src/semmle/javascript/DefUse.qll
@@ -12,6 +12,7 @@ import javascript
 * <tr><td><code>x = y</code><td><code>x = y</code><td><code>x</code><td><code>y</code></tr>
 * <tr><td><code>var a = b</code><td><code>var a = b</code><td><code>a</code><td><code>b</code></tr>
 * <tr><td><code>function f { ... }</code><td><code>f</code><td><code>f</code><td><code>function f { ... }</code></tr>
+ * <tr><td><code>function f ( x = y ){ ... }</code><td><code>x</code><td><code>x</code><td><code>y</code></tr>
 * <tr><td><code>class C { ... }</code><td><code>C</code><td><code>C</code><td><code>class C { ... }</code></tr>
 * <tr><td><code>namespace N { ... }</code><td><code>N</code><td><code>N</code><td><code>namespace N { ... }</code></tr>
 * <tr><td><code>enum E { ... }</code><td><code>E</code><td><code>E</code><td><code>enum E { ... }</code></tr>
@@ -42,6 +43,8 @@ private predicate defn(ControlFlowNode def, Expr lhs, AST::ValueNode rhs) {
  exists(EnumMember member | def = member.getIdentifier() |
    lhs = def and rhs = member.getInitializer()
  )
+  or
+  lhs = def and def.(Parameter).getDefault() = rhs
 }

 /**
@@ -187,9 +190,23 @@ class VarDef extends ControlFlowNode {
   * the value that this definition assigns to its target.
   *
   * This predicate is not defined for `VarDef`s where the source is implicit,
-   * such as `for-in` loops or parameters.
+   * such as `for-in` loops, parameters or destructuring assignments.
   */
-  AST::ValueNode getSource() { defn(this, _, result) }
+  AST::ValueNode getSource() {
+    exists(Expr target |
+      not target instanceof DestructuringPattern and defn(this, target, result)
+    )
+  }
+
+  /**
+   * Gets the source that this definition destructs, that is, the
+   * right hand side of a destructuring assignment.
+   */
+  AST::ValueNode getDestructuringSource() {
+    exists(Expr target |
+      target instanceof DestructuringPattern and defn(this, target, result)
+    )
+  }

  /**
   * Holds if this definition of `v` is overwritten by another definition, that is,
--- a/javascript/ql/src/semmle/javascript/RangeAnalysis.qll
+++ b/javascript/ql/src/semmle/javascript/RangeAnalysis.qll
@@ -104,7 +104,12 @@ module RangeAnalysis {
  pragma[noinline]
  private predicate hasUniquePredecessor(DataFlow::Node node) {
    isRelevant(node) and
-    strictcount(node.getAPredecessor()) = 1
+    strictcount(node.getAPredecessor()) = 1 and
+    // exclude parameters with default values
+    not exists (Parameter p |
+      DataFlow::parameterNode(p) = node and
+      exists(p.getDefault())
+    )
  }

  /**
--- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll
@@ -992,7 +992,9 @@ module DataFlow {
   * flow through IIFE calls into account.
   */
  private AST::ValueNode defSourceNode(VarDef def) {
-    result = def.getSource() or localArgumentPassing(result, def)
+    result = def.getSource() or
+    result = def.getDestructuringSource() or
+    localArgumentPassing(result, def)
  }

  /**
@@ -1002,15 +1004,19 @@ module DataFlow {
  private Node defSourceNode(VarDef def, SsaSourceVariable v) {
    exists(BindingPattern lhs, VarRef r |
      lhs = def.getTarget() and r = lhs.getABindingVarRef() and r.getVariable() = v
-    |
+      |
      // follow one step of the def-use chain if the lhs is a simple variable reference
      lhs = r and
      result = TValueNode(defSourceNode(def))
      or
      // handle destructuring assignments
-      exists(PropertyPattern pp | r = pp.getValuePattern() | result = TPropNode(pp))
+      exists(PropertyPattern pp | r = pp.getValuePattern() |
+        result = TPropNode(pp) or result = pp.getDefault().flow()
+      )
      or
      result = TElementPatternNode(_, r)
+      or
+      exists(ArrayPattern ap, int i | ap.getElement(i) = r and result = ap.getDefault(i).flow())
    )
  }

--- a/javascript/ql/src/semmle/javascript/dataflow/internal/VariableTypeInference.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/VariableTypeInference.qll
@@ -53,9 +53,7 @@ private class SsaVarAccessWithNonLocalAnalysis extends SsaVarAccessAnalysis {
    exists(VarDef varDef |
      varDef = def.(SsaExplicitDefinition).getDef() and
      varDef.getSource().flow() = src and
-      src instanceof CallWithNonLocalAnalyzedReturnFlow and
-      // avoid relating `v` and `f()` in `var {v} = f();`
-      not varDef.getTarget() instanceof DestructuringPattern
+      src instanceof CallWithNonLocalAnalyzedReturnFlow
    )
  }

@@ -145,7 +143,7 @@ private class AnalyzedParameter extends AnalyzedVarDef, @vardecl {

  override DataFlow::AnalyzedNode getRhs() {
    getFunction().argumentPassing(this, result.asExpr()) or
-    result = this.(Parameter).getDefault().analyze()
+    result = AnalyzedVarDef.super.getRhs()
  }

  override AbstractValue getAnRhsValue() {
--- a/javascript/ql/test/library-tests/DataFlow/flowStep.expected
+++ b/javascript/ql/test/library-tests/DataFlow/flowStep.expected
@@ -121,6 +121,25 @@
 | tst.js:103:4:103:16 | [ 19, 23, 0 ] | tst.js:98:11:98:24 | [ x, ...rest ] |
 | tst.js:105:1:105:1 | x | tst.js:105:1:105:6 | x ?? y |
 | tst.js:105:6:105:6 | y | tst.js:105:1:105:6 | x ?? y |
+| tst.js:107:2:113:1 | functio ...  v2c;\\n} | tst.js:107:1:113:2 | (functi ... v2c;\\n}) |
+| tst.js:108:6:108:38 | v1a | tst.js:109:2:109:4 | v1a |
+| tst.js:108:6:108:38 | v1b | tst.js:109:8:109:10 | v1b |
+| tst.js:108:6:108:38 | v1c | tst.js:109:14:109:16 | v1c |
+| tst.js:108:7:108:9 | v1a | tst.js:108:6:108:38 | v1a |
+| tst.js:108:12:108:20 | v1b = o1b | tst.js:108:6:108:38 | v1b |
+| tst.js:108:18:108:20 | o1b | tst.js:108:6:108:38 | v1b |
+| tst.js:108:23:108:31 | v1c = o1c | tst.js:108:6:108:38 | v1c |
+| tst.js:108:29:108:31 | o1c | tst.js:108:6:108:38 | v1c |
+| tst.js:108:36:108:38 | o1d | tst.js:108:6:108:32 | {v1a, v ...  = o1c} |
+| tst.js:111:6:111:38 | v2a | tst.js:112:2:112:4 | v2a |
+| tst.js:111:6:111:38 | v2b | tst.js:112:8:112:10 | v2b |
+| tst.js:111:6:111:38 | v2c | tst.js:112:14:112:16 | v2c |
+| tst.js:111:7:111:9 | v2a | tst.js:111:6:111:38 | v2a |
+| tst.js:111:12:111:14 | v2b | tst.js:111:6:111:38 | v2b |
+| tst.js:111:18:111:20 | o2b | tst.js:111:6:111:38 | v2b |
+| tst.js:111:23:111:25 | v2c | tst.js:111:6:111:38 | v2c |
+| tst.js:111:29:111:31 | o2c | tst.js:111:6:111:38 | v2c |
+| tst.js:111:36:111:38 | o2d | tst.js:111:6:111:32 | [v2a, v ...  = o2c] |
 | tst.ts:1:1:1:1 | A | tst.ts:1:11:1:11 | A |
 | tst.ts:1:1:1:1 | A | tst.ts:7:1:7:0 | A |
 | tst.ts:1:1:5:1 | A | tst.ts:7:1:7:0 | A |
--- a/javascript/ql/test/library-tests/DataFlow/incomplete.expected
+++ b/javascript/ql/test/library-tests/DataFlow/incomplete.expected
@@ -44,6 +44,18 @@
 | tst.js:98:11:98:24 | x | heap |
 | tst.js:99:7:99:18 | y | heap |
 | tst.js:101:3:101:16 | z | heap |
+| tst.js:108:6:108:38 | v1a | heap |
+| tst.js:108:6:108:38 | v1b | heap |
+| tst.js:108:6:108:38 | v1c | heap |
+| tst.js:108:18:108:20 | o1b | global |
+| tst.js:108:29:108:31 | o1c | global |
+| tst.js:108:36:108:38 | o1d | global |
+| tst.js:111:6:111:38 | v2a | heap |
+| tst.js:111:6:111:38 | v2b | heap |
+| tst.js:111:6:111:38 | v2c | heap |
+| tst.js:111:18:111:20 | o2b | global |
+| tst.js:111:29:111:31 | o2c | global |
+| tst.js:111:36:111:38 | o2d | global |
 | tst.ts:2:14:2:19 | x | namespace |
 | tst.ts:3:3:3:8 | setX() | call |
 | tst.ts:8:3:8:5 | A.x | heap |
--- a/javascript/ql/test/library-tests/DataFlow/sources.expected
+++ b/javascript/ql/test/library-tests/DataFlow/sources.expected
@@ -69,6 +69,20 @@
 | tst.js:99:9:99:9 | y |
 | tst.js:101:7:101:7 | z |
 | tst.js:103:4:103:16 | [ 19, 23, 0 ] |
+| tst.js:107:2:107:1 | this |
+| tst.js:107:2:113:1 | functio ...  v2c;\\n} |
+| tst.js:108:7:108:9 | v1a |
+| tst.js:108:12:108:20 | v1b = o1b |
+| tst.js:108:18:108:20 | o1b |
+| tst.js:108:23:108:31 | v1c = o1c |
+| tst.js:108:29:108:31 | o1c |
+| tst.js:108:36:108:38 | o1d |
+| tst.js:111:7:111:9 | v2a |
+| tst.js:111:12:111:14 | v2b |
+| tst.js:111:18:111:20 | o2b |
+| tst.js:111:23:111:25 | v2c |
+| tst.js:111:29:111:31 | o2c |
+| tst.js:111:36:111:38 | o2d |
 | tst.ts:1:1:1:0 | this |
 | tst.ts:3:3:3:8 | setX() |
 | tst.ts:7:1:7:0 | this |
--- a/javascript/ql/test/library-tests/DataFlow/tst.js
+++ b/javascript/ql/test/library-tests/DataFlow/tst.js
@@ -103,4 +103,12 @@ var vs2 = ( for (v of o) v );     // generator comprehensions are not analysed
 })([ 19, 23, 0 ]);

 x ?? y;                           // flow through short-circuiting operator
+
+(function(){
+	var {v1a, v1b = o1b, v1c = o1c} = o1d;
+	v1a + v1b + v1c;
+
+	var [v2a, v2b = o2b, v2c = o2c] = o2d;
+	v2a + v2b + v2c;
+});
 // semmle-extractor-options: --experimental
--- a/javascript/ql/test/library-tests/DefUse/DefUsePair.expected
+++ b/javascript/ql/test/library-tests/DefUse/DefUsePair.expected
@@ -30,3 +30,6 @@
 | tst.js:7:4:7:6 | --i | tst.js:7:6:7:6 | i |
 | tst.js:12:2:12:7 | x = 42 | tst.js:14:9:14:9 | x |
 | tst.js:19:11:19:11 | x | tst.js:18:9:18:9 | x |
+| tst.js:23:6:23:23 | {a = b, c = d} = e | tst.js:24:2:24:2 | a |
+| tst.js:23:6:23:23 | {a = b, c = d} = e | tst.js:24:6:24:6 | c |
+| tst.js:26:11:26:11 | a | tst.js:27:2:27:2 | a |
--- a/javascript/ql/test/library-tests/DefUse/VarDefDestructuringSource.expected
+++ b/javascript/ql/test/library-tests/DefUse/VarDefDestructuringSource.expected
@@ -0,0 +1 @@
+| tst.js:23:6:23:23 | {a = b, c = d} = e | tst.js:23:23:23:23 | e |
--- a/javascript/ql/test/library-tests/DefUse/VarDefDestructuringSource.ql
+++ b/javascript/ql/test/library-tests/DefUse/VarDefDestructuringSource.ql
@@ -0,0 +1,4 @@
+import javascript
+
+from VarDef d
+select d, d.getDestructuringSource()
--- a/javascript/ql/test/library-tests/DefUse/VarDefSource.expected
+++ b/javascript/ql/test/library-tests/DefUse/VarDefSource.expected
@@ -29,3 +29,4 @@
 | tst.js:11:11:11:11 | g | tst.js:11:2:15:1 | functio ... rn x;\\n} |
 | tst.js:12:2:12:7 | x = 42 | tst.js:12:6:12:7 | 42 |
 | tst.js:19:11:19:11 | x | tst.js:19:2:19:16 | function x() {} |
+| tst.js:26:11:26:11 | a | tst.js:26:15:26:15 | b |
--- a/javascript/ql/test/library-tests/DefUse/tst.js
+++ b/javascript/ql/test/library-tests/DefUse/tst.js
@@ -18,3 +18,11 @@ function f(o) {
 	return x;
 	function x() {}
 });
+
+(function(){
+	var {a = b, c = d} = e;
+	a + c;
+});
+(function(a = b) {
+	a;
+});
--- a/javascript/ql/test/query-tests/Statements/UselessComparisonTest/UselessComparisonTest.expected
+++ b/javascript/ql/test/query-tests/Statements/UselessComparisonTest/UselessComparisonTest.expected
@@ -1,3 +1,5 @@
 | constant.js:2:7:2:11 | 1 > 2 | The condition '1 > 2' is always false. |
 | constant.js:3:7:3:11 | 1 > 0 | The condition '1 > 0' is always true. |
 | example.js:8:7:8:13 | i < end | The condition 'i < end' is always false. |
+| tst.js:8:22:8:32 | i == 100000 | The condition 'i == 100000' is always false. |
+| tst.js:9:23:9:33 | i == 100000 | The condition 'i == 100000' is always false. |
--- a/javascript/ql/test/query-tests/Statements/UselessComparisonTest/tst.js
+++ b/javascript/ql/test/query-tests/Statements/UselessComparisonTest/tst.js
@@ -0,0 +1,10 @@
+(function(p = 1) {
+	if (p > 0) {
+	} else {
+	}
+});
+
+(function(){
+	(function (i) { if (i == 100000) return; })(1);
+	(function f(i) { if (i == 100000) return; f(i+1); })(1);
+});
--- a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditionalGood.js
+++ b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditionalGood.js
@@ -99,3 +99,30 @@ function getLastLine(input) {
    f10(undefined, undefined);

 })();
+
+(function(){
+	function g(p) {
+		return function() {
+			if (p) { // OK, whitelisted
+				g(p);
+			}
+		};
+	}
+
+	function f(p = false) {
+		return function() {
+			if (p) { // OK, whitelisted
+				f(p);
+			}
+		};
+	}
+
+	function h(p = false) {
+		(function() {
+			if (p) { // OK, whitelisted
+
+			}
+		});
+	}
+	h();
+});
				`@@ -0,0 +1 @@`
				`\| tst.js:23:6:23:23 \| {a = b, c = d} = e \| tst.js:23:23:23:23 \| e \|`