Merge pull request github#15489 from hvitved/csharp/lambda-field-flow

hvitved · web-flow · commit 9634511ac54a · 2024-02-12T09:14:21.000+01:00
C#: Additional tracking of lambdas through fields and properties
diff --git a/csharp/ql/consistency-queries/DataFlowConsistency.ql b/csharp/ql/consistency-queries/DataFlowConsistency.ql
@@ -7,15 +7,6 @@ private import codeql.dataflow.internal.DataFlowImplConsistency
 private module Input implements InputSig<CsharpDataFlow> {
   private import CsharpDataFlow
 
-  predicate uniqueEnclosingCallableExclude(Node n) {
-    // TODO: Remove once static initializers are folded into the
-    // static constructors
-    exists(ControlFlow::Node cfn |
-      cfn.getAstNode() = any(FieldOrProperty f | f.isStatic()).getAChild+() and
-      cfn = n.getControlFlowNode()
-    )
-  }
-
   predicate uniqueCallEnclosingCallableExclude(DataFlowCall call) {
     // TODO: Remove once static initializers are folded into the
     // static constructors
@@ -30,6 +21,8 @@ private module Input implements InputSig<CsharpDataFlow> {
     n instanceof ParameterNode
     or
     missingLocationExclude(n)
+    or
+    n instanceof FlowInsensitiveFieldNode
   }
 
   predicate missingLocationExclude(Node n) {
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll
@@ -98,7 +98,8 @@ private module Cached {
   cached
   newtype TDataFlowCallable =
     TDotNetCallable(DotNet::Callable c) { c.isUnboundDeclaration() } or
-    TSummarizedCallable(DataFlowSummarizedCallable sc)
+    TSummarizedCallable(DataFlowSummarizedCallable sc) or
+    TFieldOrProperty(FieldOrProperty f)
 
   cached
   newtype TDataFlowCall =
@@ -247,22 +248,33 @@ class ImplicitCapturedReturnKind extends ReturnKind, TImplicitCapturedReturnKind
 
 /** A callable used for data flow. */
 class DataFlowCallable extends TDataFlowCallable {
-  /** Get the underlying source code callable, if any. */
+  /** Gets the underlying source code callable, if any. */
   DotNet::Callable asCallable() { this = TDotNetCallable(result) }
 
-  /** Get the underlying summarized callable, if any. */
+  /** Gets the underlying summarized callable, if any. */
   FlowSummary::SummarizedCallable asSummarizedCallable() { this = TSummarizedCallable(result) }
 
-  /** Get the underlying callable. */
+  /** Gets the underlying field or property, if any. */
+  FieldOrProperty asFieldOrProperty() { this = TFieldOrProperty(result) }
+
+  /** Gets the underlying callable. */
   DotNet::Callable getUnderlyingCallable() {
     result = this.asCallable() or result = this.asSummarizedCallable()
   }
 
   /** Gets a textual representation of this dataflow callable. */
-  string toString() { result = this.getUnderlyingCallable().toString() }
+  string toString() {
+    result = this.getUnderlyingCallable().toString()
+    or
+    result = this.asFieldOrProperty().toString()
+  }
 
   /** Get the location of this dataflow callable. */
-  Location getLocation() { result = this.getUnderlyingCallable().getLocation() }
+  Location getLocation() {
+    result = this.getUnderlyingCallable().getLocation()
+    or
+    result = this.asFieldOrProperty().getLocation()
+  }
 }
 
 /** A call relevant for data flow. */
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll
@@ -69,13 +69,26 @@ abstract class NodeImpl extends Node {
   abstract string toStringImpl();
 }
 
+// TODO: Remove once static initializers are folded into the
+// static constructors
+private DataFlowCallable getEnclosingStaticFieldOrProperty(Expr e) {
+  result.asFieldOrProperty() =
+    any(FieldOrProperty f |
+      f.isStatic() and
+      e = f.getAChild+() and
+      not exists(e.getEnclosingCallable())
+    )
+}
+
 private class ExprNodeImpl extends ExprNode, NodeImpl {
   override DataFlowCallable getEnclosingCallableImpl() {
     result.asCallable() =
       [
         this.getExpr().(CIL::Expr).getEnclosingCallable().(DotNet::Callable),
         this.getControlFlowNodeImpl().getEnclosingCallable()
       ]
+    or
+    result = getEnclosingStaticFieldOrProperty(this.asExpr())
   }
 
   override DotNet::Type getTypeImpl() {
@@ -911,7 +924,8 @@ private module Cached {
     TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
     TParamsArgumentNode(ControlFlow::Node callCfn) {
       callCfn = any(Call c | isParamsArg(c, _, _)).getAControlFlowNode()
-    }
+    } or
+    TFlowInsensitiveFieldNode(FieldOrProperty f) { f.isFieldLike() }
 
   /**
    * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local
@@ -1021,6 +1035,8 @@ predicate nodeIsHidden(Node n) {
   n instanceof ParamsArgumentNode
   or
   n.asExpr() = any(WithExpr we).getInitializer()
+  or
+  n instanceof FlowInsensitiveFieldNode
 }
 
 /** A CIL SSA definition, viewed as a node in a data flow graph. */
@@ -1346,6 +1362,8 @@ private module ArgumentNodes {
 
     override DataFlowCallable getEnclosingCallableImpl() {
       result.asCallable() = cfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(cfn.getAstNode())
     }
 
     override Type getTypeImpl() { result = cfn.getAstNode().(Expr).getType() }
@@ -1385,6 +1403,8 @@ private module ArgumentNodes {
 
     override DataFlowCallable getEnclosingCallableImpl() {
       result.asCallable() = callCfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(callCfn.getAstNode())
     }
 
     override Type getTypeImpl() { result = this.getParameter().getType() }
@@ -1784,6 +1804,30 @@ private class FieldOrPropertyRead extends FieldOrPropertyAccess, AssignableRead
   }
 }
 
+/**
+ * A data flow node used for control-flow insensitive flow through fields
+ * and properties.
+ *
+ * In global data flow this is used to model flow through static fields and
+ * properties, while for lambda flow we additionally use it to track assignments
+ * in constructors to uses within the same class.
+ */
+class FlowInsensitiveFieldNode extends NodeImpl, TFlowInsensitiveFieldNode {
+  private FieldOrProperty f;
+
+  FlowInsensitiveFieldNode() { this = TFlowInsensitiveFieldNode(f) }
+
+  override DataFlowCallable getEnclosingCallableImpl() { result.asFieldOrProperty() = f }
+
+  override Type getTypeImpl() { result = f.getType() }
+
+  override ControlFlow::Node getControlFlowNodeImpl() { none() }
+
+  override Location getLocationImpl() { result = f.getLocation() }
+
+  override string toStringImpl() { result = "[flow-insensitive] " + f }
+}
+
 /**
  * Holds if `pred` can flow to `succ`, by jumping from one callable to
  * another. Additional steps specified by the configuration are *not*
@@ -1792,13 +1836,16 @@ private class FieldOrPropertyRead extends FieldOrPropertyAccess, AssignableRead
 predicate jumpStep(Node pred, Node succ) {
   pred.(NonLocalJumpNode).getAJumpSuccessor(true) = succ
   or
-  exists(FieldOrProperty fl, FieldOrPropertyRead flr |
-    fl.isStatic() and
-    fl.isFieldLike() and
-    fl.getAnAssignedValue() = pred.asExpr() and
-    fl.getAnAccess() = flr and
-    flr = succ.asExpr() and
-    flr.hasNonlocalValue()
+  exists(FieldOrProperty f | f.isStatic() |
+    f.getAnAssignedValue() = pred.asExpr() and
+    succ = TFlowInsensitiveFieldNode(f)
+    or
+    exists(FieldOrPropertyRead fr |
+      pred = TFlowInsensitiveFieldNode(f) and
+      f.getAnAccess() = fr and
+      fr = succ.asExpr() and
+      fr.hasNonlocalValue()
+    )
   )
   or
   FlowSummaryImpl::Private::Steps::summaryJumpStep(pred.(FlowSummaryNode).getSummaryNode(),
@@ -2258,6 +2305,8 @@ module PostUpdateNodes {
 
     override DataFlowCallable getEnclosingCallableImpl() {
       result.asCallable() = cfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(oc)
     }
 
     override DotNet::Type getTypeImpl() { result = oc.getType() }
@@ -2289,6 +2338,8 @@ module PostUpdateNodes {
 
     override DataFlowCallable getEnclosingCallableImpl() {
       result.asCallable() = cfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(cfn.getAstNode())
     }
 
     override Type getTypeImpl() { result = cfn.getAstNode().(Expr).getType() }
@@ -2437,6 +2488,24 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves
     nodeTo.asExpr().(EventRead).getTarget() = aee.getTarget() and
     preservesValue = false
   )
+  or
+  preservesValue = true and
+  exists(FieldOrProperty f, FieldOrPropertyAccess fa |
+    fa = f.getAnAccess() and
+    fa.targetIsLocalInstance()
+  |
+    exists(AssignableDefinition def |
+      def.getTargetAccess() = fa and
+      nodeFrom.asExpr() = def.getSource() and
+      nodeTo = TFlowInsensitiveFieldNode(f) and
+      nodeFrom.getEnclosingCallable() instanceof Constructor
+    )
+    or
+    nodeFrom = TFlowInsensitiveFieldNode(f) and
+    f.getAnAccess() = fa and
+    fa = nodeTo.asExpr() and
+    fa.(FieldOrPropertyRead).hasNonlocalValue()
+  )
 }
 
 /**
diff --git a/csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.cs b/csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.cs
@@ -128,9 +128,31 @@ public unsafe void M18()
     void M19(Action a, bool b)
     {
         if (b)
-            a = () => {};
+            a = () => { };
         a();
     }
 
-    void M20(bool b) => M19(() => {}, b);
+    void M20(bool b) => M19(() => { }, b);
+
+    Action<int> Field;
+    Action<int> Prop2 { get; set; }
+
+    DelegateFlow(Action<int> a, Action<int> b)
+    {
+        Field = a;
+        Prop2 = b;
+    }
+
+    void M20()
+    {
+        new DelegateFlow(
+            _ => { },
+            _ => { }
+        );
+
+        this.Field(0);
+        this.Prop2(0);
+        Field(0);
+        Prop2(0);
+    }
 }
diff --git a/csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.expected b/csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.expected
@@ -22,8 +22,12 @@ delegateCall
 | DelegateFlow.cs:89:35:89:37 | delegate call | DelegateFlow.cs:93:13:93:21 | (...) => ... |
 | DelegateFlow.cs:114:9:114:16 | function pointer call | DelegateFlow.cs:7:17:7:18 | M2 |
 | DelegateFlow.cs:125:9:125:25 | function pointer call | DelegateFlow.cs:7:17:7:18 | M2 |
-| DelegateFlow.cs:132:9:132:11 | delegate call | DelegateFlow.cs:131:17:131:24 | (...) => ... |
-| DelegateFlow.cs:132:9:132:11 | delegate call | DelegateFlow.cs:135:29:135:36 | (...) => ... |
+| DelegateFlow.cs:132:9:132:11 | delegate call | DelegateFlow.cs:131:17:131:25 | (...) => ... |
+| DelegateFlow.cs:132:9:132:11 | delegate call | DelegateFlow.cs:135:29:135:37 | (...) => ... |
+| DelegateFlow.cs:153:9:153:21 | delegate call | DelegateFlow.cs:149:13:149:20 | (...) => ... |
+| DelegateFlow.cs:154:9:154:21 | delegate call | DelegateFlow.cs:150:13:150:20 | (...) => ... |
+| DelegateFlow.cs:155:9:155:16 | delegate call | DelegateFlow.cs:149:13:149:20 | (...) => ... |
+| DelegateFlow.cs:156:9:156:16 | delegate call | DelegateFlow.cs:150:13:150:20 | (...) => ... |
 viableLambda
 | DelegateFlow.cs:9:9:9:12 | delegate call | DelegateFlow.cs:16:9:16:20 | call to method M2 | DelegateFlow.cs:16:12:16:19 | (...) => ... |
 | DelegateFlow.cs:9:9:9:12 | delegate call | DelegateFlow.cs:17:9:17:14 | call to method M2 | DelegateFlow.cs:5:10:5:11 | M1 |
@@ -49,7 +53,11 @@ viableLambda
 | DelegateFlow.cs:89:35:89:37 | delegate call | DelegateFlow.cs:93:9:93:22 | call to local function M14 | DelegateFlow.cs:93:13:93:21 | (...) => ... |
 | DelegateFlow.cs:114:9:114:16 | function pointer call | DelegateFlow.cs:119:9:119:28 | call to method M16 | DelegateFlow.cs:7:17:7:18 | M2 |
 | DelegateFlow.cs:125:9:125:25 | function pointer call | file://:0:0:0:0 | (none) | DelegateFlow.cs:7:17:7:18 | M2 |
-| DelegateFlow.cs:132:9:132:11 | delegate call | DelegateFlow.cs:135:25:135:40 | call to method M19 | DelegateFlow.cs:135:29:135:36 | (...) => ... |
-| DelegateFlow.cs:132:9:132:11 | delegate call | file://:0:0:0:0 | (none) | DelegateFlow.cs:131:17:131:24 | (...) => ... |
+| DelegateFlow.cs:132:9:132:11 | delegate call | DelegateFlow.cs:135:25:135:41 | call to method M19 | DelegateFlow.cs:135:29:135:37 | (...) => ... |
+| DelegateFlow.cs:132:9:132:11 | delegate call | file://:0:0:0:0 | (none) | DelegateFlow.cs:131:17:131:25 | (...) => ... |
+| DelegateFlow.cs:153:9:153:21 | delegate call | file://:0:0:0:0 | (none) | DelegateFlow.cs:149:13:149:20 | (...) => ... |
+| DelegateFlow.cs:154:9:154:21 | delegate call | file://:0:0:0:0 | (none) | DelegateFlow.cs:150:13:150:20 | (...) => ... |
+| DelegateFlow.cs:155:9:155:16 | delegate call | file://:0:0:0:0 | (none) | DelegateFlow.cs:149:13:149:20 | (...) => ... |
+| DelegateFlow.cs:156:9:156:16 | delegate call | file://:0:0:0:0 | (none) | DelegateFlow.cs:150:13:150:20 | (...) => ... |
 | file://:0:0:0:0 | [summary] call to [summary param] position 0 in Lazy in Lazy | DelegateFlow.cs:105:9:105:24 | object creation of type Lazy<Int32> | DelegateFlow.cs:104:23:104:30 | (...) => ... |
 | file://:0:0:0:0 | [summary] call to [summary param] position 0 in Lazy in Lazy | DelegateFlow.cs:107:9:107:24 | object creation of type Lazy<Int32> | DelegateFlow.cs:106:13:106:20 | (...) => ... |
