JS: Fix post-update flow into 'this'

Author: asgerf

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll

@@ -30,8 +30,8 @@ private module Cached {
     TValueNode(AST::ValueNode nd) or
     /** An SSA node from the legacy SSA library */
     TSsaDefNode(SsaDefinition d) or
-    /** Use of a variable with flow from a post-update node (from an earlier use) */
-    TSsaUseNode(VarUse use) { use.getVariable() instanceof PurelyLocalVariable } or
+    /** Use of a variable or 'this', with flow from a post-update node (from an earlier use) */
+    TSsaUseNode(Expr use) { use = any(Ssa2::SsaConfig::SourceVariable v).getAnAccess() } or
     /** Phi-read node (new SSA library). Ordinary phi nodes are represented by TSsaDefNode. */
     TSsaPhiReadNode(Ssa2::PhiReadNode phi) or
     /** Input to a phi node (new SSA library) */

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

@@ -5,6 +5,7 @@ private import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps
 private import semmle.javascript.dataflow.internal.AdditionalFlowInternal
 private import semmle.javascript.dataflow.internal.Contents::Private
 private import semmle.javascript.dataflow.internal.VariableCapture
+private import semmle.javascript.dataflow.internal.VariableOrThis
 private import semmle.javascript.dataflow.internal.sharedlib.DataFlowImplCommon as DataFlowImplCommon
 private import semmle.javascript.dataflow.internal.sharedlib.Ssa as Ssa2
 private import semmle.javascript.internal.flow_summaries.AllFlowSummaries
@@ -20,18 +21,18 @@ private class Node = DataFlow::Node;
 class PostUpdateNode = DataFlow::PostUpdateNode;
 
 class SsaUseNode extends DataFlow::Node, TSsaUseNode {
-  private VarAccess access;
+  private Expr expr;
 
-  SsaUseNode() { this = TSsaUseNode(access) }
+  SsaUseNode() { this = TSsaUseNode(expr) }
 
   cached
-  override string toString() { result = "[ssa-use] " + access.toString() }
+  override string toString() { result = "[ssa-use] " + expr.toString() }
 
   cached
-  override StmtContainer getContainer() { result = access.getContainer() }
+  override StmtContainer getContainer() { result = expr.getContainer() }
 
   cached
-  override Location getLocation() { result = access.getLocation() }
+  override Location getLocation() { result = expr.getLocation() }
 }
 
 class SsaPhiReadNode extends DataFlow::Node, TSsaPhiReadNode {
@@ -1056,9 +1057,9 @@ predicate knownSourceModel(Node sink, string model) { none() }
 predicate knownSinkModel(Node sink, string model) { none() }
 
 private predicate samePhi(SsaPhiNode legacyPhi, Ssa2::PhiNode newPhi) {
-  exists(BasicBlock bb, PurelyLocalVariable v |
+  exists(BasicBlock bb, LocalVariableOrThis v |
     newPhi.definesAt(v, bb, _) and
-    legacyPhi.definesAt(bb, _, v)
+    legacyPhi.definesAt(bb, _, v.asLocalVariable())
   )
 }
 
@@ -1082,10 +1083,11 @@ private predicate useUseFlow(Node node1, Node node2) {
   exists(Ssa2::DefinitionExt def, Ssa2::Node ssa1, Ssa2::Node ssa2, boolean isUseStep |
     Ssa2::localFlowStep(def, ssa1, ssa2, isUseStep) and
     node1 = getNodeFromSsa2(ssa1) and
-    node2 = getNodeFromSsa2(ssa2)
+    node2 = getNodeFromSsa2(ssa2) and
+    not node1.getTopLevel().isExterns()
   )
   or
-  exists(VarUse use |
+  exists(Expr use |
     node1 = TSsaUseNode(use) and
     node2 = TValueNode(use)
   )

javascript/ql/lib/semmle/javascript/dataflow/internal/VariableOrThis.qll

@@ -0,0 +1,76 @@
+private import javascript
+
+cached
+private newtype TLocalVariableOrThis =
+  TLocalVariable(LocalVariable var) or
+  TThis(StmtContainer container) { not container instanceof ArrowFunctionExpr }
+
+/** A local variable or `this` in a particular container. */
+class LocalVariableOrThis extends TLocalVariableOrThis {
+  /** Gets the local variable represented by this newtype, if any. */
+  LocalVariable asLocalVariable() { this = TLocalVariable(result) }
+
+  /** If this represents `this`, gets the enclosing container */
+  StmtContainer asThisContainer() { this = TThis(result) }
+
+  /** Gets the name of the variable or the string `"this"`. */
+  string toString() { result = this.getName() }
+
+  /** Gets the name of the variable or the string `"this"`. */
+  string getName() {
+    result = this.asLocalVariable().getName()
+    or
+    this instanceof TThis and result = "this"
+  }
+
+  /** Gets the location of a declaration of this variable, or the declaring container if this is `this`. */
+  DbLocation getLocation() {
+    result = this.asLocalVariable().getLocation()
+    or
+    result = this.asThisContainer().getLocation()
+  }
+
+  /** Holds if this is a captured variable or captured `this`. */
+  predicate isCaptured() {
+    this.asLocalVariable().isCaptured()
+    or
+    hasCapturedThis(this.asThisContainer())
+  }
+
+  /** Gets the container declaring this variable or is the enclosing container for `this`. */
+  StmtContainer getDeclaringContainer() {
+    result = this.asLocalVariable().getDeclaringContainer()
+    or
+    result = this.asThisContainer()
+  }
+
+  /** Gets an access to `this` represented by this value. */
+  ThisExpr getAThisAccess() { result.getBindingContainer() = this.asThisContainer() }
+
+  /** Gets an access to variable or `this`. */
+  Expr getAnAccess() {
+    result = this.asLocalVariable().getAnAccess()
+    or
+    result = this.getAThisAccess()
+  }
+}
+
+bindingset[c1, c2]
+pragma[inline_late]
+private predicate sameContainer(StmtContainer c1, StmtContainer c2) { c1 = c2 }
+
+pragma[nomagic]
+private predicate hasCapturedThis(StmtContainer c) {
+  exists(ThisExpr expr |
+    expr.getBindingContainer() = c and
+    not sameContainer(c, expr.getContainer())
+  )
+}
+
+module LocalVariableOrThis {
+  /** Gets the representation of the given local variable. */
+  LocalVariableOrThis variable(LocalVariable v) { result.asLocalVariable() = v }
+
+  /** Gets the representation of `this` in the given container. */
+  LocalVariableOrThis thisInContainer(StmtContainer c) { result = TThis(c) }
+}

javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll

@@ -7,8 +7,9 @@
 private import javascript as js
 private import codeql.ssa.Ssa
 private import semmle.javascript.internal.BasicBlockInternal as BasicBlockInternal
+private import semmle.javascript.dataflow.internal.VariableOrThis
 
-private module SsaConfig implements InputSig<js::DbLocation> {
+module SsaConfig implements InputSig<js::DbLocation> {
   class ControlFlowNode = js::ControlFlowNode;
 
   class BasicBlock = js::BasicBlock;
@@ -17,7 +18,9 @@ private module SsaConfig implements InputSig<js::DbLocation> {
     ExitBasicBlock() { this.isExitBlock() }
   }
 
-  class SourceVariable = js::PurelyLocalVariable; // TODO: include 'this' as it is relevant for use-use flow
+  class SourceVariable extends LocalVariableOrThis {
+    SourceVariable() { not this.isCaptured() }
+  }
 
   pragma[nomagic]
   private js::EntryBasicBlock getEntryBlock(js::StmtContainer container) {
@@ -27,7 +30,7 @@ private module SsaConfig implements InputSig<js::DbLocation> {
   predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain) {
     certain = true and
     (
-      bb.defAt(i, v, _)
+      bb.defAt(i, v.asLocalVariable(), _)
       or
       // Implicit initialization and function parameters
       bb = getEntryBlock(v.getDeclaringContainer()) and
@@ -36,7 +39,11 @@ private module SsaConfig implements InputSig<js::DbLocation> {
   }
 
   predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) {
-    bb.useAt(i, v, _) and certain = true
+    bb.useAt(i, v.asLocalVariable(), _) and certain = true
+    or
+    certain = true and
+    bb.getNode(i) = v.getAThisAccess()
+    // TODO: also account for: super() and field initialisers
   }
 
   predicate getImmediateBasicBlockDominator = BasicBlockInternal::immediateDominator/1;
@@ -48,7 +55,9 @@ private module SsaConfig implements InputSig<js::DbLocation> {
 import Make<js::DbLocation, SsaConfig>
 
 private module SsaDataflowInput implements DataFlowIntegrationInputSig {
-  class Expr extends js::VarUse {
+  class Expr extends js::Expr {
+    Expr() { this = any(SsaConfig::SourceVariable v).getAnAccess() }
+
     predicate hasCfgNode(js::BasicBlock bb, int i) { this = bb.getNode(i) }
   }
 

javascript/ql/test/library-tests/TripleDot/useuse.js

@@ -45,7 +45,7 @@ function t4() {
     class C {
         constructor(x) {
             this.foo = x;
-            sink(this.foo); // $ MISSING: hasValueFlow=t4.1
+            sink(this.foo); // $ hasValueFlow=t4.1
         }
     }
     const c = new C(source('t4.1'));