Merge pull request github#10933 from hvitved/csharp/fix-flow-into-phis

C#: Fix flow steps into phi/uncertain def nodes

Author: hvitved

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

@@ -310,12 +310,27 @@ module LocalFlow {
    * Holds if `nodeFrom` is a last node referencing SSA definition `def`, which
    * can reach `next`.
    */
-  private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) {
-    exists(ControlFlow::BasicBlock bb, int i | SsaImpl::lastRefBeforeRedef(def, bb, i, next) |
+  private predicate localFlowSsaInputFromDef(
+    Node nodeFrom, Ssa::Definition def, Ssa::Definition next
+  ) {
+    exists(ControlFlow::BasicBlock bb, int i |
+      SsaImpl::lastRefBeforeRedef(def, bb, i, next) and
       def.definesAt(_, bb, i) and
       def = getSsaDefinition(nodeFrom)
-      or
-      nodeFrom.asExprAtNode(bb.getNode(i)) instanceof AssignableRead
+    )
+  }
+
+  /**
+   * Holds if `read` is a last node reading SSA definition `def`, which
+   * can reach `next`.
+   */
+  predicate localFlowSsaInputFromExpr(
+    ControlFlow::Node read, Ssa::Definition def, Ssa::Definition next
+  ) {
+    exists(ControlFlow::BasicBlock bb, int i |
+      SsaImpl::lastRefBeforeRedef(def, bb, i, next) and
+      read = bb.getNode(i) and
+      read.getElement() instanceof AssignableRead
     )
   }
 
@@ -351,18 +366,14 @@ module LocalFlow {
     // Flow from read to next read
     localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
     or
-    // Flow into phi node
-    exists(Ssa::PhiNode phi |
-      localFlowSsaInput(nodeFrom, def, phi) and
-      phi = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = phi.getAnInput()
-    )
-    or
-    // Flow into uncertain SSA definition
-    exists(LocalFlow::UncertainExplicitSsaDefinition uncertain |
-      localFlowSsaInput(nodeFrom, def, uncertain) and
-      uncertain = nodeTo.(SsaDefinitionNode).getDefinition() and
-      def = uncertain.getPriorDefinition()
+    // Flow into phi/uncertain SSA definition node from def
+    exists(Ssa::Definition next |
+      localFlowSsaInputFromDef(nodeFrom, def, next) and
+      next = nodeTo.(SsaDefinitionNode).getDefinition()
+    |
+      def = next.(Ssa::PhiNode).getAnInput()
+      or
+      def = next.(LocalFlow::UncertainExplicitSsaDefinition).getPriorDefinition()
     )
   }
 
@@ -519,11 +530,26 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   or
   exists(Ssa::Definition def |
     LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
-    not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom,
-      any(DataFlowSummarizedCallable sc)) and
+    not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) and
     not LocalFlow::usesInstanceField(def)
   )
   or
+  // Flow into phi/uncertain SSA definition node from read
+  exists(Ssa::Definition def, ControlFlow::Node read, Ssa::Definition next |
+    LocalFlow::localFlowSsaInputFromExpr(read, def, next) and
+    next = nodeTo.(SsaDefinitionNode).getDefinition() and
+    def =
+      [
+        next.(Ssa::PhiNode).getAnInput(),
+        next.(LocalFlow::UncertainExplicitSsaDefinition).getPriorDefinition()
+      ]
+  |
+    exists(nodeFrom.asExprAtNode(read)) and
+    not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
+    or
+    exists(nodeFrom.(PostUpdateNode).getPreUpdateNode().asExprAtNode(read))
+  )
+  or
   LocalFlow::localFlowCapturedVarStep(nodeFrom, nodeTo)
   or
   FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true)
@@ -859,6 +885,21 @@ private module Cached {
       LocalFlow::usesInstanceField(def)
     )
     or
+    // Flow into phi/uncertain SSA definition node from read
+    exists(Ssa::Definition def, ControlFlow::Node read, Ssa::Definition next |
+      LocalFlow::localFlowSsaInputFromExpr(read, def, next) and
+      next = nodeTo.(SsaDefinitionNode).getDefinition() and
+      def =
+        [
+          next.(Ssa::PhiNode).getAnInput(),
+          next.(LocalFlow::UncertainExplicitSsaDefinition).getPriorDefinition()
+        ]
+    |
+      exists(nodeFrom.asExprAtNode(read))
+      or
+      exists(nodeFrom.(PostUpdateNode).getPreUpdateNode().asExprAtNode(read))
+    )
+    or
     // Simple flow through library code is included in the exposed local
     // step relation, even though flow is technically inter-procedural
     FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo,

csharp/ql/test/library-tests/dataflow/fields/FieldFlow.expected

@@ -918,6 +918,14 @@ edges
 | J.cs:106:14:106:15 | access to local variable a3 [property X] : Object | J.cs:106:14:106:17 | access to property X |
 | J.cs:107:14:107:15 | access to local variable a3 [property Y] : Object | J.cs:107:14:107:17 | access to property Y |
 | J.cs:107:14:107:15 | access to local variable a3 [property Y] : Object | J.cs:107:14:107:17 | access to property Y |
+| J.cs:119:13:119:13 | [post] access to local variable a [element] : Int32 | J.cs:125:14:125:14 | access to local variable a [element] : Int32 |
+| J.cs:119:13:119:13 | [post] access to local variable a [element] : Int32 | J.cs:125:14:125:14 | access to local variable a [element] : Int32 |
+| J.cs:119:20:119:34 | call to method Source<Int32> : Int32 | J.cs:119:13:119:13 | [post] access to local variable a [element] : Int32 |
+| J.cs:119:20:119:34 | call to method Source<Int32> : Int32 | J.cs:119:13:119:13 | [post] access to local variable a [element] : Int32 |
+| J.cs:125:14:125:14 | access to local variable a [element] : Int32 | J.cs:125:14:125:17 | access to array element : Int32 |
+| J.cs:125:14:125:14 | access to local variable a [element] : Int32 | J.cs:125:14:125:17 | access to array element : Int32 |
+| J.cs:125:14:125:17 | access to array element : Int32 | J.cs:125:14:125:17 | (...) ... |
+| J.cs:125:14:125:17 | access to array element : Int32 | J.cs:125:14:125:17 | (...) ... |
 nodes
 | A.cs:5:17:5:28 | call to method Source<C> : C | semmle.label | call to method Source<C> : C |
 | A.cs:5:17:5:28 | call to method Source<C> : C | semmle.label | call to method Source<C> : C |
@@ -1925,6 +1933,16 @@ nodes
 | J.cs:107:14:107:15 | access to local variable a3 [property Y] : Object | semmle.label | access to local variable a3 [property Y] : Object |
 | J.cs:107:14:107:17 | access to property Y | semmle.label | access to property Y |
 | J.cs:107:14:107:17 | access to property Y | semmle.label | access to property Y |
+| J.cs:119:13:119:13 | [post] access to local variable a [element] : Int32 | semmle.label | [post] access to local variable a [element] : Int32 |
+| J.cs:119:13:119:13 | [post] access to local variable a [element] : Int32 | semmle.label | [post] access to local variable a [element] : Int32 |
+| J.cs:119:20:119:34 | call to method Source<Int32> : Int32 | semmle.label | call to method Source<Int32> : Int32 |
+| J.cs:119:20:119:34 | call to method Source<Int32> : Int32 | semmle.label | call to method Source<Int32> : Int32 |
+| J.cs:125:14:125:14 | access to local variable a [element] : Int32 | semmle.label | access to local variable a [element] : Int32 |
+| J.cs:125:14:125:14 | access to local variable a [element] : Int32 | semmle.label | access to local variable a [element] : Int32 |
+| J.cs:125:14:125:17 | (...) ... | semmle.label | (...) ... |
+| J.cs:125:14:125:17 | (...) ... | semmle.label | (...) ... |
+| J.cs:125:14:125:17 | access to array element : Int32 | semmle.label | access to array element : Int32 |
+| J.cs:125:14:125:17 | access to array element : Int32 | semmle.label | access to array element : Int32 |
 subpaths
 | A.cs:6:24:6:24 | access to local variable c : C | A.cs:147:32:147:32 | c : C | A.cs:149:20:149:27 | object creation of type B [field c] : C | A.cs:6:17:6:25 | call to method Make [field c] : C |
 | A.cs:6:24:6:24 | access to local variable c : C | A.cs:147:32:147:32 | c : C | A.cs:149:20:149:27 | object creation of type B [field c] : C | A.cs:6:17:6:25 | call to method Make [field c] : C |
@@ -2101,3 +2119,4 @@ subpaths
 | J.cs:102:14:102:17 | access to property X | J.cs:97:17:97:33 | call to method Source<Object> : Object | J.cs:102:14:102:17 | access to property X | $@ | J.cs:97:17:97:33 | call to method Source<Object> : Object | call to method Source<Object> : Object |
 | J.cs:106:14:106:17 | access to property X | J.cs:97:17:97:33 | call to method Source<Object> : Object | J.cs:106:14:106:17 | access to property X | $@ | J.cs:97:17:97:33 | call to method Source<Object> : Object | call to method Source<Object> : Object |
 | J.cs:107:14:107:17 | access to property Y | J.cs:105:32:105:48 | call to method Source<Object> : Object | J.cs:107:14:107:17 | access to property Y | $@ | J.cs:105:32:105:48 | call to method Source<Object> : Object | call to method Source<Object> : Object |
+| J.cs:125:14:125:17 | (...) ... | J.cs:119:20:119:34 | call to method Source<Int32> : Int32 | J.cs:125:14:125:17 | (...) ... | $@ | J.cs:119:20:119:34 | call to method Source<Int32> : Int32 | call to method Source<Int32> : Int32 |

csharp/ql/test/library-tests/dataflow/fields/J.cs

@@ -111,6 +111,35 @@ private void M5()
         Sink(a4.Y); // no flow
     }
 
+    private void M6(bool b)
+    {
+        var a = new int[1];
+        if (b)
+        {
+            a[0] = Source<int>(10);
+        }
+        else
+        {
+            a = new int[1];
+        }
+        Sink(a[0]); // $ hasValueFlow=10
+    }
+
+    private void M7(bool b)
+    {
+        var a = new System.Collections.Generic.List<int>();
+        if (b)
+        {
+            a.Add(Source<int>(11));
+            a.Clear();
+        }
+        else
+        {
+            a = new System.Collections.Generic.List<int>();
+        }
+        Sink(a[0]);
+    }
+
     public static void Sink(object o) { }
 
     static T Source<T>(object source) => throw null;