Merge pull request #1686 from asger-semmle/lvalue-node

Approved by xiemaisi

javascript/ql/src/semmle/javascript/RangeAnalysis.qll

@@ -98,29 +98,6 @@ module RangeAnalysis {
     )
   }
 
-  /**
-   * Holds if the given node has a unique data flow predecessor.
-   */
-  pragma[noinline]
-  private predicate hasUniquePredecessor(DataFlow::Node node) {
-    isRelevant(node) and
-    strictcount(node.getAPredecessor()) = 1 and
-    // exclude parameters with default values
-    not exists(Parameter p |
-      DataFlow::parameterNode(p) = node and
-      exists(p.getDefault())
-    )
-  }
-
-  /**
-   * Gets the definition of `node`, without unfolding phi nodes.
-   */
-  DataFlow::Node getDefinition(DataFlow::Node node) {
-    if hasUniquePredecessor(node)
-    then result = getDefinition(node.getAPredecessor())
-    else result = node
-  }
-
   /**
    * Gets a data flow node holding the result of the add/subtract operation in
    * the given increment/decrement expression.
@@ -229,8 +206,8 @@ module RangeAnalysis {
    * Holds if `r` can be modelled as `r = root * sign + bias`.
    */
   predicate linearDefinition(DataFlow::Node r, DataFlow::Node root, int sign, Bias bias) {
-    if hasUniquePredecessor(r)
-    then linearDefinition(r.getAPredecessor(), root, sign, bias)
+    if exists(r.getImmediatePredecessor())
+    then linearDefinition(r.getImmediatePredecessor(), root, sign, bias)
     else
       if linearDefinitionStep(r, _, _, _)
       then
@@ -257,8 +234,8 @@ module RangeAnalysis {
   predicate linearDefinitionSum(
     DataFlow::Node r, DataFlow::Node xroot, int xsign, DataFlow::Node yroot, int ysign, Bias bias
   ) {
-    if hasUniquePredecessor(r)
-    then linearDefinitionSum(r.getAPredecessor(), xroot, xsign, yroot, ysign, bias)
+    if exists(r.getImmediatePredecessor())
+    then linearDefinitionSum(r.getImmediatePredecessor(), xroot, xsign, yroot, ysign, bias)
     else
       if exists(r.asExpr().getIntValue())
       then none() // do not model constants as sums
@@ -336,7 +313,8 @@ module RangeAnalysis {
     ConditionGuardNode guard, DataFlow::Node a, int asign, string operator, DataFlow::Node b,
     int bsign, Bias bias
   ) {
-    exists(Comparison compare | compare = getDefinition(guard.getTest().flow()).asExpr() |
+    exists(Comparison compare |
+      compare = guard.getTest().flow().getImmediatePredecessor*().asExpr() and
       linearComparison(compare, a, asign, b, bsign, bias) and
       (
         guard.getOutcome() = true and operator = compare.getOperator()

javascript/ql/src/semmle/javascript/SSA.qll

@@ -527,7 +527,7 @@ class SsaExplicitDefinition extends SsaDefinition, TExplicitDef {
    * if any.
    */
   DataFlow::Node getRhsNode() {
-    result = DataFlow::defSourceNode(getDef(), getSourceVariable())
+    result = DataFlow::ssaDefinitionNode(this).getImmediatePredecessor()
   }
 }
 

javascript/ql/src/semmle/javascript/Stmt.qll

@@ -818,6 +818,17 @@ abstract class EnhancedForLoop extends LoopStmt {
     result = getIterator().(DeclStmt).getADecl()
   }
 
+  /**
+   * Gets the property, variable, or destructuring pattern occurring as the iterator
+   * expression in this `for`-`in` or `for`-`of` loop.
+   */
+  Expr getLValue() {
+    result = getIterator() and
+    (result instanceof BindingPattern or result instanceof PropAccess)
+    or
+    result = getIterator().(DeclStmt).getADecl().getBindingPattern()
+  }
+
   /**
    * Gets an iterator variable of this `for`-`in` or `for`-`of` loop.
    */

javascript/ql/src/semmle/javascript/StringConcatenation.qll

@@ -9,7 +9,7 @@ module StringConcatenation {
   private DataFlow::Node getAssignAddResult(AssignAddExpr expr) {
     result = expr.flow()
     or
-    result = DataFlow::ssaDefinitionNode(SSA::definition(expr))
+    result = DataFlow::lvalueNode(expr.getTarget())
   }
 
   /** Gets the `n`th operand to the string concatenation defining `node`. */

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

@@ -166,59 +166,31 @@ module DataFlow {
      * Gets the immediate predecessor of this node, if any.
      *
      * A node with an immediate predecessor can usually only have the value that flows
-     * into its from its immediate predecessor, currently with two exceptions:
-     *
-     * - An immediately-invoked function expression with a single return expression `e`
-     *   has `e` as its immediate predecessor, even if the function can fall over the
-     *   end and return `undefined`.
-     *
-     * - A destructuring property pattern or element pattern with a default value has
-     *   both the `PropRead` and its default value as immediate predecessors.
+     * into its from its immediate predecessor.
      */
     cached
     DataFlow::Node getImmediatePredecessor() {
+      lvalueFlowStep(result, this) and
+      not lvalueDefaultFlowStep(_, this)
+      or
       // Use of variable -> definition of variable
       exists(SsaVariable var |
-        this = DataFlow::valueNode(var.getAUse()) and
-        result.(DataFlow::SsaDefinitionNode).getSsaVariable() = var
+        this = valueNode(var.getAUse()) and
+        result = TSsaDefNode(var)
       )
       or
       // Refinement of variable -> original definition of variable
       exists(SsaRefinementNode refinement |
-        this.(DataFlow::SsaDefinitionNode).getSsaVariable() = refinement.getVariable() and
-        result.(DataFlow::SsaDefinitionNode).getSsaVariable() = refinement.getAnInput()
-      )
-      or
-      // Definition of variable -> RHS of definition
-      exists(SsaExplicitDefinition def |
-        this = TSsaDefNode(def) and
-        result = def.getRhsNode()
+        this = TSsaDefNode(refinement) and
+        result = TSsaDefNode(refinement.getAnInput())
       )
       or
       // IIFE call -> return value of IIFE
-      // Note: not sound in case function falls over end and returns 'undefined'
       exists(Function fun |
         localCall(this.asExpr(), fun) and
         result = fun.getAReturnedExpr().flow() and
-        strictcount(fun.getAReturnedExpr()) = 1
-      )
-      or
-      // IIFE parameter -> IIFE call
-      exists(Parameter param |
-        this = DataFlow::parameterNode(param) and
-        localArgumentPassing(result.asExpr(), param)
-      )
-      or
-      // `{ x } -> e` in `let { x } = e`
-      exists(DestructuringPattern pattern |
-        this = TDestructuringPatternNode(pattern)
-      |
-        exists(VarDef def |
-          pattern = def.getTarget() and
-          result = DataFlow::valueNode(def.getDestructuringSource())
-        )
-        or
-        result = patternPropRead(pattern)
+        strictcount(fun.getAReturnedExpr()) = 1 and
+        not fun.getExit().isJoin() // can only reach exit by the return statement
       )
     }
   }
@@ -1106,11 +1078,7 @@ module DataFlow {
    * INTERNAL: Use `parameterNode(Parameter)` instead.
    */
   predicate parameterNode(DataFlow::Node nd, Parameter p) {
-    nd = ssaDefinitionNode(SSA::definition((SimpleParameter)p))
-    or
-    nd = TDestructuringPatternNode(p)
-    or
-    nd = TUnusedParameterNode(p)
+    nd = lvalueNode(p)
   }
 
   /**
@@ -1150,24 +1118,22 @@ module DataFlow {
   }
 
   /**
-   * INTERNAL. DO NOT USE.
+   * Gets the data flow node corresponding the given l-value expression, if
+   * such a node exists.
    *
-   * Gets the `PropRead` node corresponding to the value stored in the given
-   * binding pattern due to destructuring.
-   *
-   * For example, in `let { p: value } = f()`, the `value` pattern maps to a `PropRead`
-   * extracting the `p` property.
+   * This differs from `DataFlow::valueNode()`, which represents the value
+   * _before_ the l-value is assigned to, whereas `DataFlow::lvalueNode()`
+   * represents the value _after_ the assignment.
    */
-  private DataFlow::PropRead patternPropRead(BindingPattern value) {
-    exists(PropertyPattern prop |
-      value = prop.getValuePattern() and
-      result = TPropNode(prop)
+  Node lvalueNode(BindingPattern lvalue) {
+    exists(SsaExplicitDefinition ssa |
+      ssa.defines(lvalue.(LValue).getDefNode(), lvalue.(VarRef).getVariable()) and
+      result = TSsaDefNode(ssa)
     )
     or
-    exists(ArrayPattern array |
-      value = array.getAnElement() and
-      result = TElementPatternNode(array, value)
-    )
+    result = TDestructuringPatternNode(lvalue)
+    or
+    result = TUnusedParameterNode(lvalue)
   }
 
   /**
@@ -1212,18 +1178,60 @@ module DataFlow {
     any(ImmediatelyInvokedFunctionExpr iife).argumentPassing(parm, arg)
   }
 
+  /**
+   * Holds if there is a step from `pred -> succ` due to an assignment
+   * to an expression in l-value position.
+   */
+  private predicate lvalueFlowStep(Node pred, Node succ) {
+    exists(VarDef def |
+      pred = valueNode(defSourceNode(def)) and
+      succ = lvalueNode(def.getTarget())
+    )
+    or
+    exists(PropertyPattern pattern |
+      pred = TPropNode(pattern) and
+      succ = lvalueNode(pattern.getValuePattern())
+    )
+    or
+    exists(Expr element |
+      pred = TElementPatternNode(_, element) and
+      succ = lvalueNode(element)
+    )
+  }
+
+  /**
+   * Holds if there is a step from `pred -> succ` from the default
+   * value of a destructuring pattern or parameter.
+   */
+  private predicate lvalueDefaultFlowStep(Node pred, Node succ) {
+    exists(PropertyPattern pattern |
+      pred = valueNode(pattern.getDefault()) and
+      succ = lvalueNode(pattern.getValuePattern())
+    )
+    or
+    exists(ArrayPattern array, int i |
+      pred = valueNode(array.getDefault(i)) and
+      succ = lvalueNode(array.getElement(i))
+    )
+    or
+    exists(Parameter param |
+      pred = valueNode(param.getDefault()) and
+      succ = parameterNode(param)
+    )
+  }
+
   /**
    * Holds if data can flow from `pred` to `succ` in one local step.
    */
   cached
   predicate localFlowStep(Node pred, Node succ) {
-    // flow into local variables
-    exists(SsaDefinition ssa | succ = TSsaDefNode(ssa) |
-      // from the rhs of an explicit definition into the variable
-      exists(SsaExplicitDefinition def | def = ssa |
-        pred = defSourceNode(def.getDef(), def.getSourceVariable())
-      )
-      or
+    // flow from RHS into LHS
+    lvalueFlowStep(pred, succ)
+    or
+    lvalueDefaultFlowStep(pred, succ)
+    or
+    // Flow through implicit SSA nodes
+    exists(SsaImplicitDefinition ssa | succ = TSsaDefNode(ssa) |
       // from any explicit definition or implicit init of a captured variable into
       // the capturing definition
       exists(SsaSourceVariable v, SsaDefinition predDef |
@@ -1270,29 +1278,6 @@ module DataFlow {
       )
     )
     or
-    exists(VarDef def |
-      // from `e` to `{ p: x }` in `{ p: x } = e`
-      pred = valueNode(defSourceNode(def)) and
-      succ = TDestructuringPatternNode(def.getTarget())
-    )
-    or
-    // flow from the value read from a property pattern to the value being
-    // destructured in the child pattern. For example, for
-    //
-    //   let { p: { q: x } } = obj
-    //
-    // add edge from the 'p:' pattern to '{ q:x }'.
-    exists(PropertyPattern pattern |
-      pred = TPropNode(pattern) and
-      succ = TDestructuringPatternNode(pattern.getValuePattern())
-    )
-    or
-    // Like the step above, but for array destructuring patterns.
-    exists(Expr elm |
-      pred = TElementPatternNode(_, elm) and
-      succ = TDestructuringPatternNode(elm)
-    )
-    or
     // flow from 'this' parameter into 'this' expressions
     exists(ThisExpr thiz |
       pred = TThisNode(thiz.getBindingContainer()) and
@@ -1323,31 +1308,6 @@ module DataFlow {
     localArgumentPassing(result, def)
   }
 
-  /**
-   * INTERNAL. DO NOT USE.
-   *
-   * Gets the data flow node representing the source of the definition of `v` at `def`,
-   * if any.
-   */
-  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) 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())
-    )
-  }
-
   /**
    * Holds if the flow information for this node is incomplete.
    *

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

@@ -232,7 +232,7 @@ module TaintTracking {
       exists(ForOfStmt fos |
         this = DataFlow::valueNode(fos.getIterationDomain()) and
         pred = this and
-        succ = DataFlow::ssaDefinitionNode(SSA::definition(fos.getIteratorExpr()))
+        succ = DataFlow::lvalueNode(fos.getLValue())
       )
     }
   }

javascript/ql/test/library-tests/GlobalAccessPaths/GlobalAccessPaths.expected

@@ -51,11 +51,12 @@ test_fromReference
 | test.js:24:9:24:16 | NS \|\| {} | NS |
 | test.js:26:1:26:8 | Conflict | Conflict |
 | test.js:33:7:33:18 | { bar = {} } | foo |
-| test.js:33:7:33:24 | bar | foo.bar |
 | test.js:33:9:33:16 | bar = {} | foo.bar |
 | test.js:33:22:33:24 | foo | foo |
-| test.js:34:11:34:13 | bar | foo.bar |
-| test.js:34:11:34:17 | bar.baz | foo.bar.baz |
+| test.js:39:3:39:20 | lazyInit | foo.bar |
+| test.js:39:14:39:16 | foo | foo |
+| test.js:39:14:39:20 | foo.bar | foo.bar |
+| test.js:40:3:40:10 | lazyInit | foo.bar |
 test_fromRhs
 | other_ns.js:4:9:4:16 | NS \|\| {} | NS |
 | other_ns.js:6:12:6:13 | {} | Conflict |
@@ -65,8 +66,10 @@ test_fromRhs
 | test.js:28:1:28:20 | class GlobalClass {} | GlobalClass |
 | test.js:30:1:30:28 | functio ... on() {} | globalFunction |
 | test.js:32:1:35:1 | functio ... .baz'\\n} | destruct |
+| test.js:37:1:41:1 | functio ... Init;\\n} | lazy |
 test_assignedUnique
 | GlobalClass |
 | destruct |
 | f |
 | globalFunction |
+| lazy |

javascript/ql/test/library-tests/GlobalAccessPaths/test.js

@@ -33,3 +33,9 @@ function destruct() {
   let { bar = {} } = foo;
   let v = bar.baz; // 'foo.bar.baz'
 }
+
+function lazy() {
+  var lazyInit;
+  lazyInit = foo.bar; // 'foo.bar'
+  lazyInit;
+}

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -3,6 +3,7 @@ typeInferenceMismatch
 | addexpr.js:4:10:4:17 | source() | addexpr.js:6:3:6:14 | x |
 | addexpr.js:11:15:11:22 | source() | addexpr.js:17:5:17:18 | value |
 | addexpr.js:11:15:11:22 | source() | addexpr.js:19:3:19:14 | value |
+| destruct.js:20:7:20:14 | source() | destruct.js:13:14:13:19 | [a, b] |
 #select
 | access-path-sanitizer.js:2:18:2:25 | source() | access-path-sanitizer.js:4:8:4:12 | obj.x |
 | addexpr.js:4:10:4:17 | source() | addexpr.js:7:8:7:8 | x |
@@ -38,9 +39,11 @@ typeInferenceMismatch
 | constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:30:8:30:19 | d_safe.taint |
 | constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:17:8:17:14 | c.param |
 | constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:25:8:25:14 | d.param |
-| destruct.js:15:7:15:14 | source() | destruct.js:5:10:5:10 | z |
-| destruct.js:15:7:15:14 | source() | destruct.js:8:10:8:10 | w |
-| destruct.js:15:7:15:14 | source() | destruct.js:11:10:11:10 | q |
+| destruct.js:20:7:20:14 | source() | destruct.js:5:10:5:10 | z |
+| destruct.js:20:7:20:14 | source() | destruct.js:8:10:8:10 | w |
+| destruct.js:20:7:20:14 | source() | destruct.js:11:10:11:10 | q |
+| destruct.js:20:7:20:14 | source() | destruct.js:14:12:14:12 | a |
+| destruct.js:20:7:20:14 | source() | destruct.js:15:12:15:12 | b |
 | exceptions.js:3:15:3:22 | source() | exceptions.js:5:10:5:10 | e |
 | exceptions.js:21:17:21:24 | source() | exceptions.js:23:10:23:10 | e |
 | exceptions.js:21:17:21:24 | source() | exceptions.js:24:10:24:21 | e.toString() |

javascript/ql/test/library-tests/TaintTracking/destruct.js

@@ -9,6 +9,11 @@ function test() {
     
     let { x: [ { y: q } ] } = obj;
     sink(q); // NOT OK
+
+    for (let [a, b] of obj) {
+      sink(a); // NOT OK
+      sink(b); // NOT OK
+    }
   }
   
   function g() {

javascript/ql/test/query-tests/Statements/UselessComparisonTest/defaults.js

@@ -0,0 +1,11 @@
+function defaultParam(param = 0) {
+  if (param > 0) {} // OK
+}
+
+function defaultPattern(obj, arr) {
+  let { prop = 0 } = obj;
+  if (prop > 0) {} // OK
+
+  let [ elm = 0 ] = arr;
+  if (elm > 0) {} // OK
+}

javascript/ql/test/query-tests/Statements/UselessComparisonTest/implicitReturn.js

@@ -0,0 +1,6 @@
+function test() {
+  let x = (function() {
+    if (g) return 5;
+  })();
+  if (x + 1 < 5) {} // OK
+}