Merge pull request github#4557 from hvitved/csharp/dataflow/parameters

C#: Simpler data-flow modelling of parameters

Author: hvitved

csharp/ql/src/semmle/code/cil/Declaration.qll

@@ -24,9 +24,6 @@ class Declaration extends DotNet::Declaration, Element, @cil_declaration {
 
   override Declaration getUnboundDeclaration() { result = this }
 
-  /** Holds if this declaration is a source declaration. */
-  final predicate isUnboundDeclaration() { this = getUnboundDeclaration() }
-
   /**
    * DEPRECATED: Use `isUnboundDeclaration()` instead.
    *

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

@@ -16,9 +16,6 @@ private import TypeRef
 class Declaration extends DotNet::Declaration, Element, @declaration {
   override ValueOrRefType getDeclaringType() { none() }
 
-  /** Holds if this declaration is unbound. */
-  final predicate isUnboundDeclaration() { this = this.getUnboundDeclaration() }
-
   /** Holds if this declaration is unconstructed and in source code. */
   final predicate isSourceDeclaration() { this.fromSource() and this.isUnboundDeclaration() }
 

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

@@ -391,7 +391,7 @@ class SummaryDelegateCall extends DelegateDataFlowCall, TSummaryDelegateCall {
 
   override DataFlowCallable getARuntimeTarget(CallContext::CallContext cc) {
     exists(SummaryDelegateParameterSink p |
-      p = TSummaryParameterNode(c, pos) and
+      p.isParameterOf(c, pos) and
       result = p.getARuntimeTarget(cc)
     )
   }

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

@@ -278,14 +278,20 @@ module LocalFlow {
     )
   }
 
+  private Ssa::Definition getSsaDefinition(Node n) {
+    result = n.(SsaDefinitionNode).getDefinition()
+    or
+    result = n.(ExplicitParameterNode).getSsaDefinition()
+  }
+
   /**
    * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving
    * SSA definition `def.
    */
   predicate localSsaFlowStep(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
-    // Flow from SSA definition to first read
+    // Flow from SSA definition/parameter to first read
     exists(ControlFlow::Node cfn |
-      def = nodeFrom.(SsaDefinitionNode).getDefinition() and
+      def = getSsaDefinition(nodeFrom) and
       nodeTo.asExprAtNode(cfn) = def.getAFirstReadAtNode(cfn)
     )
     or
@@ -323,11 +329,10 @@ module LocalFlow {
     )
   }
 
-  predicate localFlowCapturedVarStep(SsaDefinitionNode nodeFrom, ImplicitCapturedArgumentNode nodeTo) {
+  predicate localFlowCapturedVarStep(Node nodeFrom, ImplicitCapturedArgumentNode nodeTo) {
     // Flow from SSA definition to implicit captured variable argument
     exists(Ssa::ExplicitDefinition def, ControlFlow::Nodes::ElementNode call |
-      def = nodeFrom.getDefinition()
-    |
+      def = getSsaDefinition(nodeFrom) and
       def.isCapturedVariableDefinitionFlowIn(_, call, _) and
       nodeTo = TImplicitCapturedArgumentNode(call, def.getSourceVariable().getAssignable())
     )
@@ -569,9 +574,15 @@ private module Cached {
       Stages::DataFlowStage::forceCachingInSameStage() and cfn.getElement() instanceof Expr
     } or
     TCilExprNode(CIL::Expr e) { e.getImplementation() instanceof CIL::BestImplementation } or
-    TSsaDefinitionNode(Ssa::Definition def) or
-    TInstanceParameterNode(Callable c) { c.hasBody() and not c.(Modifiable).isStatic() } or
-    TCilParameterNode(CIL::MethodParameter p) { p.getMethod().hasBody() } or
+    TSsaDefinitionNode(Ssa::Definition def) {
+      // Handled by `TExplicitParameterNode` below
+      not def.(Ssa::ExplicitDefinition).getADefinition() instanceof
+        AssignableDefinitions::ImplicitParameterDefinition
+    } or
+    TExplicitParameterNode(DotNet::Parameter p) { p.isUnboundDeclaration() } or
+    TInstanceParameterNode(Callable c) {
+      c.isUnboundDeclaration() and not c.(Modifiable).isStatic()
+    } or
     TYieldReturnNode(ControlFlow::Nodes::ElementNode cfn) {
       any(Callable c).canYieldReturn(cfn.getElement())
     } or
@@ -605,18 +616,6 @@ private module Cached {
         cfn.getElement() = fla.getQualifier()
       )
     } or
-    TSummaryParameterNode(SummarizedCallable c, int i) {
-      exists(SummaryInput input | FlowSummaryImpl::Private::summary(c, input, _, _, _, _) |
-        input = SummaryInput::parameter(i)
-        or
-        input = SummaryInput::delegate(i)
-      )
-      or
-      exists(SummaryOutput output |
-        FlowSummaryImpl::Private::summary(c, _, _, output, _, _) and
-        output = SummaryOutput::delegate(i, _)
-      )
-    } or
     TSummaryInternalNode(
       SummarizedCallable c, FlowSummaryImpl::Private::SummaryInternalNodeState state
     ) {
@@ -801,12 +800,10 @@ private module Cached {
   cached
   predicate isUnreachableInCall(Node n, DataFlowCall call) {
     exists(
-      SsaDefinitionNode paramNode, Ssa::ExplicitDefinition param, Guard guard,
-      ControlFlow::SuccessorTypes::BooleanSuccessor bs
+      ExplicitParameterNode paramNode, Guard guard, ControlFlow::SuccessorTypes::BooleanSuccessor bs
     |
       viableConstantBooleanParamArg(paramNode, bs.getValue().booleanNot(), call) and
-      paramNode.getDefinition() = param and
-      param.getARead() = guard and
+      paramNode.getSsaDefinition().getARead() = guard and
       guard.controlsBlock(n.getControlFlowNode().getBasicBlock(), bs, _)
     )
   }
@@ -874,6 +871,11 @@ private module Cached {
       def instanceof Ssa::ImplicitCallDefinition
     )
     or
+    exists(Parameter p |
+      p = n.(ParameterNode).getParameter() and
+      not p.fromSource()
+    )
+    or
     n instanceof YieldReturnNode
     or
     n instanceof ImplicitCapturedArgumentNode
@@ -905,33 +907,25 @@ class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode {
 
   override Location getLocationImpl() { result = def.getLocation() }
 
-  override string toStringImpl() {
-    not explicitParameterNode(this, _) and
-    result = def.toString()
-  }
+  override string toStringImpl() { result = def.toString() }
 }
 
 private module ParameterNodes {
   abstract private class ParameterNodeImpl extends ParameterNode, NodeImpl { }
 
-  /**
-   * Holds if definition node `node` is an entry definition for parameter `p`.
-   */
-  predicate explicitParameterNode(AssignableDefinitionNode node, Parameter p) {
-    p = node.getDefinition().(AssignableDefinitions::ImplicitParameterDefinition).getParameter()
-  }
-
   /**
    * The value of an explicit parameter at function entry, viewed as a node in a data
    * flow graph.
    */
-  class ExplicitParameterNode extends ParameterNodeImpl {
+  class ExplicitParameterNode extends ParameterNodeImpl, TExplicitParameterNode {
     private DotNet::Parameter parameter;
 
-    ExplicitParameterNode() {
-      explicitParameterNode(this, parameter)
-      or
-      this = TCilParameterNode(parameter)
+    ExplicitParameterNode() { this = TExplicitParameterNode(parameter) }
+
+    /** Gets the SSA definition corresponding to this parameter, if any. */
+    Ssa::ExplicitDefinition getSsaDefinition() {
+      result.getADefinition().(AssignableDefinitions::ImplicitParameterDefinition).getParameter() =
+        this.getParameter()
     }
 
     override DotNet::Parameter getParameter() { result = parameter }
@@ -1037,46 +1031,6 @@ private module ParameterNodes {
       c = this.getEnclosingCallable()
     }
   }
-
-  /** A parameter node for a callable with a flow summary. */
-  class SummaryParameterNode extends ParameterNodeImpl, SummaryNodeImpl, TSummaryParameterNode {
-    private SummarizedCallable sc;
-    private int i;
-
-    SummaryParameterNode() { this = TSummaryParameterNode(sc, i) }
-
-    override Parameter getParameter() { result = sc.getParameter(i) }
-
-    override predicate isParameterOf(DataFlowCallable c, int pos) {
-      c = sc and
-      pos = i
-    }
-
-    override Callable getEnclosingCallableImpl() { result = sc }
-
-    override Type getTypeImpl() {
-      result = sc.getParameter(i).getType()
-      or
-      i = -1 and
-      result = sc.getDeclaringType()
-    }
-
-    override ControlFlow::Node getControlFlowNodeImpl() { none() }
-
-    override Location getLocationImpl() {
-      result = sc.getParameter(i).getLocation()
-      or
-      i = -1 and
-      result = sc.getLocation()
-    }
-
-    override string toStringImpl() {
-      result = "[summary] " + sc.getParameter(i)
-      or
-      i = -1 and
-      result = "[summary] this"
-    }
-  }
 }
 
 import ParameterNodes
@@ -1934,7 +1888,7 @@ private class ConstantBooleanArgumentNode extends ExprNode {
 
 pragma[noinline]
 private predicate viableConstantBooleanParamArg(
-  SsaDefinitionNode paramNode, boolean b, DataFlowCall call
+  ParameterNode paramNode, boolean b, DataFlowCall call
 ) {
   exists(ConstantBooleanArgumentNode arg |
     viableParamArg(call, paramNode, arg) and

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

@@ -119,12 +119,10 @@ class ExprNode extends Node {
 class ParameterNode extends Node {
   ParameterNode() {
     // charpred needed to avoid making `ParameterNode` abstract
-    explicitParameterNode(this, _) or
+    this = TExplicitParameterNode(_) or
     this.(SsaDefinitionNode).getDefinition() instanceof
       ImplicitCapturedParameterNodeImpl::SsaCapturedEntryDefinition or
-    this = TInstanceParameterNode(_) or
-    this = TCilParameterNode(_) or
-    this = TSummaryParameterNode(_, _)
+    this = TInstanceParameterNode(_)
   }
 
   /** Gets the parameter corresponding to this node, if any. */

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

@@ -10,6 +10,7 @@ private import semmle.code.csharp.dataflow.CallContext
 private import semmle.code.csharp.dataflow.internal.DataFlowDispatch
 private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
 private import semmle.code.csharp.dataflow.internal.DataFlowPublic
+private import semmle.code.csharp.dataflow.FlowSummary
 private import semmle.code.csharp.dispatch.Dispatch
 private import semmle.code.csharp.frameworks.system.linq.Expressions
 
@@ -102,9 +103,10 @@ class DelegateCallExpr extends DelegateFlowSink, DataFlow::ExprNode {
 }
 
 /** A parameter of delegate type belonging to a callable with a flow summary. */
-class SummaryDelegateParameterSink extends DelegateFlowSink, SummaryParameterNode {
+class SummaryDelegateParameterSink extends DelegateFlowSink, ParameterNode {
   SummaryDelegateParameterSink() {
-    this.getType() instanceof SystemLinqExpressions::DelegateExtType
+    this.getType() instanceof SystemLinqExpressions::DelegateExtType and
+    this.isParameterOf(any(SummarizedCallable c), _)
   }
 }
 

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

@@ -87,7 +87,7 @@ module Private {
   NodeImpl inputNode(SummarizableCallable c, SummaryInput input) {
     exists(int i |
       input = TParameterSummaryInput(i) and
-      result = DataFlowPrivate::TSummaryParameterNode(c, i)
+      result.(ParameterNode).isParameterOf(c, i)
     )
     or
     exists(int i |

csharp/ql/src/semmle/code/dotnet/Declaration.qll

@@ -52,6 +52,9 @@ class Declaration extends NamedElement, @dotnet_declaration {
    * | `C<int>.Method<string>` | `C<>.Method<>`      |
    */
   Declaration getUnboundDeclaration() { result = this }
+
+  /** Holds if this declaration is unbound. */
+  final predicate isUnboundDeclaration() { this.getUnboundDeclaration() = this }
 }
 
 /** A member of a type. */

csharp/ql/test/library-tests/csharp7/LocalTaintFlow.expected

@@ -1,3 +1,4 @@
+| CSharp7.cs:7:7:7:14 | this | CSharp7.cs:9:9:9:9 | this access |
 | CSharp7.cs:9:9:9:9 | [post] this access | CSharp7.cs:10:9:10:9 | this access |
 | CSharp7.cs:9:9:9:9 | this access | CSharp7.cs:10:9:10:9 | this access |
 | CSharp7.cs:10:9:10:9 | [post] this access | CSharp7.cs:11:9:11:9 | this access |

csharp/ql/test/library-tests/dataflow/delegates/DelegateFlow.expected

@@ -1,6 +1,6 @@
 summaryDelegateCall
-| file://:0:0:0:0 | [summary] valueFactory | DelegateFlow.cs:104:23:104:30 | (...) => ... | DelegateFlow.cs:105:23:105:23 | access to local variable f |
-| file://:0:0:0:0 | [summary] valueFactory | DelegateFlow.cs:106:13:106:20 | (...) => ... | DelegateFlow.cs:107:23:107:23 | access to local variable f |
+| file://:0:0:0:0 | valueFactory | DelegateFlow.cs:104:23:104:30 | (...) => ... | DelegateFlow.cs:105:23:105:23 | access to local variable f |
+| file://:0:0:0:0 | valueFactory | DelegateFlow.cs:106:13:106:20 | (...) => ... | DelegateFlow.cs:107:23:107:23 | access to local variable f |
 delegateCall
 | DelegateFlow.cs:9:9:9:12 | delegate call | DelegateFlow.cs:5:10:5:11 | M1 | DelegateFlow.cs:17:12:17:13 | delegate creation of type Action<Int32> |
 | DelegateFlow.cs:9:9:9:12 | delegate call | DelegateFlow.cs:5:10:5:11 | M1 | DelegateFlow.cs:22:12:22:12 | access to parameter a |