C#: Nested field flow

Author: hvitved

csharp/ql/src/semmle/code/csharp/Assignable.qll

@@ -81,7 +81,7 @@ class AssignableRead extends AssignableAccess {
 
   pragma[noinline]
   private ControlFlow::Node getAnAdjacentReadSameVar() {
-    Ssa::Internal::adjacentReadPairSameVar(this.getAControlFlowNode(), result)
+    Ssa::Internal::adjacentReadPairSameVar(_, this.getAControlFlowNode(), result)
   }
 
   /**

csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll

@@ -462,7 +462,7 @@ private predicate defReaches(Ssa::Definition def, ControlFlow::Node cfn, boolean
   (always = true or always = false)
   or
   exists(ControlFlow::Node mid | defReaches(def, mid, always) |
-    Ssa::Internal::adjacentReadPairSameVar(mid, cfn) and
+    Ssa::Internal::adjacentReadPairSameVar(_, mid, cfn) and
     not mid = any(Dereference d |
         if always = true
         then d.isAlwaysNull(def.getSourceVariable())

csharp/ql/src/semmle/code/csharp/dataflow/SSA.qll

@@ -860,13 +860,15 @@ module Ssa {
       }
 
       /**
-       * Holds if the read at `cfn2` is a read of the same SSA definition as the
-       * read at `cfn1`, and `cfn2` can be reached from `cfn1` without passing
-       * through another read.
+       * Holds if the read at `cfn2` is a read of the same SSA definition `def`
+       * as the read at `cfn1`, and `cfn2` can be reached from `cfn1` without
+       * passing through another read.
        */
       cached
-      predicate adjacentReadPairSameVar(ControlFlow::Node cfn1, ControlFlow::Node cfn2) {
-        exists(TrackedDefinition def, BasicBlock bb1, int i1 |
+      predicate adjacentReadPairSameVar(
+        TrackedDefinition def, ControlFlow::Node cfn1, ControlFlow::Node cfn2
+      ) {
+        exists(BasicBlock bb1, int i1 |
           variableRead(bb1, i1, _, cfn1, _) and
           adjacentVarRead(def, bb1, i1, cfn2)
         )
@@ -1115,9 +1117,7 @@ module Ssa {
       }
 
       /** Gets a run-time target for the delegate call `c`. */
-      Callable getARuntimeDelegateTarget(Call c) {
-        delegateCall(c, delegateCallSource(result))
-      }
+      Callable getARuntimeDelegateTarget(Call c) { delegateCall(c, delegateCallSource(result)) }
     }
 
     /** Holds if `(c1,c2)` is an edge in the call graph. */

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -297,71 +297,86 @@ private module Cached {
       any(DelegateArgumentConfiguration x).hasExprPath(_, cfn, _, call)
     } or
     TMallocNode(ControlFlow::Nodes::ElementNode cfn) { cfn.getElement() instanceof ObjectCreation } or
-    TArgumentPostCallNode(ControlFlow::Nodes::ElementNode cfn) {
+    TExprPostUpdateNode(ControlFlow::Nodes::ElementNode cfn) {
       exists(Argument a, Type t |
         a = cfn.getElement() and
         t = a.stripCasts().getType()
       |
         t instanceof RefType or
         t = any(TypeParameter tp | not tp.isValueType())
       )
-    } or
-    TStoreTargetNode(ControlFlow::Nodes::ElementNode cfn) {
+      or
       instanceFieldLikeAssign(_, _, _, cfn.getElement())
+      or
+      exists(TExprPostUpdateNode upd, FieldLikeAccess fla |
+        upd = TExprPostUpdateNode(fla.getAControlFlowNode())
+      |
+        cfn.getElement() = fla.getQualifier()
+      )
     }
 
-  /**
-   * This is the local flow predicate that's used as a building block in global
-   * data flow. It may have less flow than the `localFlowStep` predicate.
-   */
+  private predicate usesInstanceField(Ssa::Definition def) {
+    exists(Ssa::SourceVariables::FieldOrPropSourceVariable fp | fp = def.getSourceVariable() |
+      not fp.getAssignable().isStatic()
+    )
+  }
+
   cached
-  predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
-    any(LocalFlow::LocalExprStepConfiguration x).hasNodePath(nodeFrom, nodeTo)
+  predicate localFlowStepImpl(Node nodeFrom, Node nodeTo, boolean simple) {
+    any(LocalFlow::LocalExprStepConfiguration x).hasNodePath(nodeFrom, nodeTo) and
+    simple = true
     or
     // Flow from SSA definition to first read
     exists(Ssa::Definition def, ControlFlow::Node cfn |
-      def = nodeFrom.(SsaDefinitionNode).getDefinition()
-    |
-      nodeTo.asExprAtNode(cfn) = def.getAFirstReadAtNode(cfn)
+      def = nodeFrom.(SsaDefinitionNode).getDefinition() and
+      nodeTo.asExprAtNode(cfn) = def.getAFirstReadAtNode(cfn) and
+      if usesInstanceField(def) then simple = false else simple = true
     )
     or
     // Flow from read to next read
-    exists(ControlFlow::Node cfnFrom, ControlFlow::Node cfnTo |
-      Ssa::Internal::adjacentReadPairSameVar(cfnFrom, cfnTo) and
-      nodeTo = TExprNode(cfnTo)
+    exists(Ssa::Definition def, ControlFlow::Node cfnFrom, ControlFlow::Node cfnTo |
+      Ssa::Internal::adjacentReadPairSameVar(def, cfnFrom, cfnTo) and
+      nodeTo = TExprNode(cfnTo) and
+      if usesInstanceField(def) then simple = false else simple = true
     |
       nodeFrom = TExprNode(cfnFrom)
       or
       cfnFrom = nodeFrom.(PostUpdateNode).getPreUpdateNode().getControlFlowNode()
     )
     or
-    ThisFlow::adjacentThisRefs(nodeFrom, nodeTo)
+    ThisFlow::adjacentThisRefs(nodeFrom, nodeTo) and
+    simple = true
     or
-    ThisFlow::adjacentThisRefs(nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
+    ThisFlow::adjacentThisRefs(nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo) and
+    simple = true
     or
     // Flow into SSA pseudo definition
     exists(Ssa::Definition def, Ssa::PseudoDefinition pseudo |
-      LocalFlow::localFlowSsaInput(nodeFrom, def)
-    |
+      LocalFlow::localFlowSsaInput(nodeFrom, def) and
       pseudo = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = pseudo.getAnInput()
+      def = pseudo.getAnInput() and
+      if usesInstanceField(def) then simple = false else simple = true
     )
     or
     // Flow into uncertain SSA definition
     exists(Ssa::Definition def, LocalFlow::UncertainExplicitSsaDefinition uncertain |
-      LocalFlow::localFlowSsaInput(nodeFrom, def)
-    |
+      LocalFlow::localFlowSsaInput(nodeFrom, def) and
       uncertain = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = uncertain.getPriorDefinition()
+      def = uncertain.getPriorDefinition() and
+      if usesInstanceField(def) then simple = false else simple = true
     )
     or
-    LocalFlow::localFlowCapturedVarStep(nodeFrom, nodeTo)
+    LocalFlow::localFlowCapturedVarStep(nodeFrom, nodeTo) and
+    simple = true
     or
-    flowOutOfDelegateLibraryCall(nodeFrom, nodeTo, true)
+    flowOutOfDelegateLibraryCall(nodeFrom, nodeTo, true) and
+    simple = true
     or
-    flowThroughLibraryCallableOutRef(_, nodeFrom, nodeTo, true)
+    flowThroughLibraryCallableOutRef(_, nodeFrom, nodeTo, true) and
+    simple = true
     or
-    LocalFlow::localFlowStepCil(nodeFrom, nodeTo)
+    LocalFlow::localFlowStepCil(nodeFrom, nodeTo) and
+    simple = true
   }
 
   /**
@@ -409,6 +424,15 @@ private module Cached {
 }
 import Cached
 
+/**
+ * This is the local flow predicate that is used as a building block in global
+ * data flow. It is a strict subset of the `localFlowStep` predicate, as it
+ * excludes SSA flow through instance fields.
+ */
+predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
+  localFlowStepImpl(nodeFrom, nodeTo, true)
+}
+
 /** An SSA definition, viewed as a node in a data flow graph. */
 class SsaDefinitionNode extends Node, TSsaDefinitionNode {
   Ssa::Definition def;
@@ -1250,26 +1274,10 @@ private module PostUpdateNodes {
     override MallocNode getPreUpdateNode() { this = TExprNode(result.getControlFlowNode()) }
   }
 
-  private class ArgumentPostCallNode extends PostUpdateNode, TArgumentPostCallNode {
-    private ControlFlow::Nodes::ElementNode cfn;
-
-    ArgumentPostCallNode() { this = TArgumentPostCallNode(cfn) }
-
-    override ExprNode getPreUpdateNode() { cfn = result.getControlFlowNode() }
-
-    override Callable getEnclosingCallable() { result = cfn.getEnclosingCallable() }
-
-    override Type getType() { result = cfn.getElement().(Expr).getType() }
-
-    override Location getLocation() { result = cfn.getLocation() }
-
-    override string toString() { result = "[post] " + cfn.toString() }
-  }
-
-  private class StoreTargetNode extends PostUpdateNode, TStoreTargetNode {
+  private class ExprPostUpdateNode extends PostUpdateNode, TExprPostUpdateNode {
     private ControlFlow::Nodes::ElementNode cfn;
 
-    StoreTargetNode() { this = TStoreTargetNode(cfn) }
+    ExprPostUpdateNode() { this = TExprPostUpdateNode(cfn) }
 
     override ExprNode getPreUpdateNode() { cfn = result.getControlFlowNode() }
 

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll

@@ -152,7 +152,7 @@ ParameterNode parameterNode(DotNet::Parameter p) { result.getParameter() = p }
  * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local
  * (intra-procedural) step.
  */
-predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFrom, nodeTo) }
+predicate localFlowStep(Node nodeFrom, Node nodeTo) { localFlowStepImpl(nodeFrom, nodeTo, _) }
 
 /**
  * Holds if data flows from `source` to `sink` in zero or more local

csharp/ql/src/semmle/code/csharp/exprs/Call.qll

@@ -425,9 +425,7 @@ class ConstructorInitializer extends Call, @constructor_init_expr {
    * }
    * ```
    */
-  predicate isThis() {
-    this.getTargetType() = this.getConstructorType()
-  }
+  predicate isThis() { this.getTargetType() = this.getConstructorType() }
 
   /**
    * Holds if this initialier is a `base` initializer, for example `base(0)`
@@ -445,9 +443,7 @@ class ConstructorInitializer extends Call, @constructor_init_expr {
    * }
    * ```
    */
-  predicate isBase() {
-    this.getTargetType() != this.getConstructorType()
-  }
+  predicate isBase() { this.getTargetType() != this.getConstructorType() }
 
   /**
    * Gets the constructor that this initializer call belongs to. For example,

csharp/ql/test/library-tests/cil/dataflow/TaintTracking.ql

@@ -1,6 +1,5 @@
 import csharp
 import semmle.code.csharp.dataflow.TaintTracking
-
 // Test that all the copies of the taint tracking library can be imported
 // simultaneously without errors.
 import semmle.code.csharp.dataflow.TaintTracking2

csharp/ql/test/library-tests/controlflow/graph/EnclosingCallable.ql

@@ -3,6 +3,4 @@ import Common
 
 query predicate nodeEnclosing(SourceControlFlowNode n, Callable c) { c = n.getEnclosingCallable() }
 
-query predicate blockEnclosing(SourceBasicBlock bb, Callable c) {
-  c = bb.getCallable()
-}
+query predicate blockEnclosing(SourceBasicBlock bb, Callable c) { c = bb.getCallable() }