C#: Additional tracking of lambdas through fields and properties

2026-04-27 09:45:15 +02:00 · 2024-01-31 11:29:54 +01:00
parent 817d04c087
commit bfe4a4bf0b
4 changed files with 105 additions and 23 deletions
--- 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) {
--- 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. */
--- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll
+++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll
@@ -69,6 +69,17 @@ 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() =
@@ -76,6 +87,8 @@ private class ExprNodeImpl extends ExprNode, NodeImpl {
        this.getExpr().(CIL::Expr).getEnclosingCallable().(DotNet::Callable),
        this.getControlFlowNodeImpl().getEnclosingCallable()
      ]
+    or
+    result = getEnclosingStaticFieldOrProperty(this.asExpr())
  }

  override DotNet::Type getTypeImpl() {
@@ -909,7 +922,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
@@ -1019,6 +1033,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. */
@@ -1344,6 +1360,8 @@ private module ArgumentNodes {

    override DataFlowCallable getEnclosingCallableImpl() {
      result.asCallable() = cfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(cfn.getAstNode())
    }

    override Type getTypeImpl() { result = cfn.getAstNode().(Expr).getType() }
@@ -1383,6 +1401,8 @@ private module ArgumentNodes {

    override DataFlowCallable getEnclosingCallableImpl() {
      result.asCallable() = callCfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(callCfn.getAstNode())
    }

    override Type getTypeImpl() { result = this.getParameter().getType() }
@@ -1782,6 +1802,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*
@@ -1790,13 +1834,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(),
@@ -2248,6 +2295,8 @@ module PostUpdateNodes {

    override DataFlowCallable getEnclosingCallableImpl() {
      result.asCallable() = cfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(oc)
    }

    override DotNet::Type getTypeImpl() { result = oc.getType() }
@@ -2279,6 +2328,8 @@ module PostUpdateNodes {

    override DataFlowCallable getEnclosingCallableImpl() {
      result.asCallable() = cfn.getEnclosingCallable()
+      or
+      result = getEnclosingStaticFieldOrProperty(cfn.getAstNode())
    }

    override Type getTypeImpl() { result = cfn.getAstNode().(Expr).getType() }
@@ -2427,6 +2478,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()
+  )
 }

 /**
--- a/csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.expected
+++ b/csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.expected
@@ -24,6 +24,10 @@ delegateCall
 | 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: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 |
@@ -51,5 +55,9 @@ viableLambda
 | 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: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 | (...) => ... |