C#: Get rid of negative parameter/argument data-flow positions

Author: hvitved

csharp/ql/lib/semmle/code/csharp/dataflow/FlowSummary.qll

@@ -40,7 +40,12 @@ module SummaryComponent {
   predicate return = SummaryComponentInternal::return/1;
 
   /** Gets a summary component that represents a qualifier. */
-  SummaryComponent qualifier() { result = argument(-1) }
+  SummaryComponent qualifier() {
+    exists(ParameterPosition pos |
+      result = SummaryComponentInternal::argument(pos) and
+      pos.isThisParameter()
+    )
+  }
 
   /** Gets a summary component that represents an element in a collection. */
   SummaryComponent element() { result = content(any(DataFlow::ElementContent c)) }
@@ -140,12 +145,17 @@ private class SummarizedCallableDefaultClearsContent extends Impl::Public::Summa
 
   // By default, we assume that all stores into arguments are definite
   override predicate clearsContent(ParameterPosition pos, DataFlow::Content content) {
-    exists(SummaryComponentStack output |
+    exists(SummaryComponentStack output, SummaryComponent target |
       this.propagatesFlow(_, output, _) and
       output.drop(_) =
         SummaryComponentStack::push(SummaryComponent::content(content),
-          SummaryComponentStack::argument(pos.getPosition())) and
+          SummaryComponentStack::singleton(target)) and
       not content instanceof DataFlow::ElementContent
+    |
+      target = SummaryComponent::argument(pos.getPosition())
+      or
+      target = SummaryComponent::qualifier() and
+      pos.isThisParameter()
     )
   }
 }

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

@@ -116,19 +116,17 @@ private module Cached {
   cached
   DataFlowCallable viableCallable(DataFlowCall call) { result = call.getARuntimeTarget() }
 
-  private int parameterPosition() {
-    result =
-      [
-        -1, any(Parameter p).getPosition(),
-        ImplicitCapturedParameterNodeImpl::getParameterPosition(_)
-      ]
-  }
-
   cached
-  newtype TParameterPosition = MkParameterPosition(int i) { i = parameterPosition() }
+  newtype TParameterPosition =
+    TPositionalParameterPosition(int i) { i = any(Parameter p).getPosition() } or
+    TThisParameterPosition() or
+    TImplicitCapturedParameterPosition(SsaCapturedEntryDefinition def)
 
   cached
-  newtype TArgumentPosition = MkArgumentPosition(int i) { i = parameterPosition() }
+  newtype TArgumentPosition =
+    TPositionalArgumentPosition(int i) { i = any(Parameter p).getPosition() } or
+    TQualifierArgumentPosition() or
+    TImplicitCapturedArgumentPosition(SsaCapturedEntryDefinition def)
 }
 
 import Cached
@@ -268,8 +266,8 @@ abstract class DataFlowCall extends TDataFlowCall {
   /** Gets the underlying expression, if any. */
   final DotNet::Expr getExpr() { result = this.getNode().asExpr() }
 
-  /** Gets the `i`th argument of this call. */
-  final ArgumentNode getArgument(int i) { result.argumentOf(this, i) }
+  /** Gets the argument at position `pos` of this call. */
+  final ArgumentNode getArgument(ArgumentPosition pos) { result.argumentOf(this, pos) }
 
   /** Gets a textual representation of this call. */
   abstract string toString();
@@ -425,36 +423,64 @@ class SummaryCall extends DelegateDataFlowCall, TSummaryCall {
   override Location getLocation() { result = c.getLocation() }
 }
 
-/** A parameter position represented by an integer. */
-class ParameterPosition extends MkParameterPosition {
-  private int i;
+/** A parameter position. */
+class ParameterPosition extends TParameterPosition {
+  /** Gets the underlying integer position, if any. */
+  int getPosition() { this = TPositionalParameterPosition(result) }
 
-  ParameterPosition() { this = MkParameterPosition(i) }
+  /** Holds if this position represents a `this` parameter. */
+  predicate isThisParameter() { this = TThisParameterPosition() }
 
-  /** Gets the underlying integer. */
-  int getPosition() { result = i }
+  /** Holds if this position is used to model flow through captured variables. */
+  predicate isImplicitCapturedParameterPosition(SsaCapturedEntryDefinition def) {
+    this = TImplicitCapturedParameterPosition(def)
+  }
 
   /** Gets a textual representation of this position. */
-  string toString() { result = i.toString() }
+  string toString() {
+    result = "position " + this.getPosition()
+    or
+    this.isThisParameter() and result = "this"
+    or
+    exists(SsaCapturedEntryDefinition def |
+      this.isImplicitCapturedParameterPosition(def) and result = "captured " + def
+    )
+  }
 }
 
-/** An argument position represented by an integer. */
-class ArgumentPosition extends MkArgumentPosition {
-  private int i;
+/** An argument position. */
+class ArgumentPosition extends TArgumentPosition {
+  /** Gets the underlying integer position, if any. */
+  int getPosition() { this = TPositionalArgumentPosition(result) }
 
-  ArgumentPosition() { this = MkArgumentPosition(i) }
+  /** Holds if this position represents a qualifier. */
+  predicate isQualifier() { this = TQualifierArgumentPosition() }
 
-  /** Gets the underlying integer. */
-  int getPosition() { result = i }
+  /** Holds if this position is used to model flow through captured variables. */
+  predicate isImplicitCapturedArgumentPosition(SsaCapturedEntryDefinition def) {
+    this = TImplicitCapturedArgumentPosition(def)
+  }
 
   /** Gets a textual representation of this position. */
-  string toString() { result = i.toString() }
+  string toString() {
+    result = "position " + this.getPosition()
+    or
+    this.isQualifier() and result = "qualifier"
+    or
+    exists(SsaCapturedEntryDefinition def |
+      this.isImplicitCapturedArgumentPosition(def) and result = "captured " + def
+    )
+  }
 }
 
 /** Holds if arguments at position `apos` match parameters at position `ppos`. */
 predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
-  exists(int i |
-    ppos = MkParameterPosition(i) and
-    apos = MkArgumentPosition(i)
+  ppos.getPosition() = apos.getPosition()
+  or
+  ppos.isThisParameter() and apos.isQualifier()
+  or
+  exists(SsaCapturedEntryDefinition def |
+    ppos.isImplicitCapturedParameterPosition(def) and
+    apos.isImplicitCapturedArgumentPosition(def)
   )
 }

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

@@ -22,13 +22,13 @@ private import semmle.code.csharp.frameworks.system.threading.Tasks
 DataFlowCallable nodeGetEnclosingCallable(Node n) { result = n.getEnclosingCallable() }
 
 /** Holds if `p` is a `ParameterNode` of `c` with position `pos`. */
-predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition pos) {
-  exists(int i | pos = MkParameterPosition(i) and p.isParameterOf(c, i))
+predicate isParameterNode(ParameterNodeImpl p, DataFlowCallable c, ParameterPosition pos) {
+  p.isParameterOf(c, pos)
 }
 
 /** Holds if `arg` is an `ArgumentNode` of `c` with position `pos`. */
 predicate isArgumentNode(ArgumentNode arg, DataFlowCall c, ArgumentPosition pos) {
-  exists(int i | pos = MkArgumentPosition(i) and arg.argumentOf(c, i))
+  arg.argumentOf(c, pos)
 }
 
 abstract class NodeImpl extends Node {
@@ -469,26 +469,28 @@ private predicate isParamsArg(Call c, Expr arg, Parameter p) {
 /** An argument of a C# call (including qualifier arguments). */
 private class Argument extends Expr {
   private Expr call;
-  private int arg;
+  private ArgumentPosition arg;
 
   Argument() {
     call =
       any(DispatchCall dc |
-        this = dc.getArgument(arg) and
+        this = dc.getArgument(arg.getPosition()) and
         not isParamsArg(_, this, _)
         or
-        this = dc.getQualifier() and arg = -1 and not dc.getAStaticTarget().(Modifiable).isStatic()
+        this = dc.getQualifier() and
+        arg.isQualifier() and
+        not dc.getAStaticTarget().(Modifiable).isStatic()
       ).getCall()
     or
-    this = call.(DelegateLikeCall).getArgument(arg)
+    this = call.(DelegateLikeCall).getArgument(arg.getPosition())
   }
 
   /**
    * Holds if this expression is the `i`th argument of `c`.
    *
    * Qualifier arguments have index `-1`.
    */
-  predicate isArgumentOf(Expr c, int i) { c = call and i = arg }
+  predicate isArgumentOf(Expr c, ArgumentPosition pos) { c = call and pos = arg }
 }
 
 /**
@@ -855,7 +857,7 @@ class SsaDefinitionNode extends NodeImpl, TSsaDefinitionNode {
 }
 
 abstract class ParameterNodeImpl extends NodeImpl {
-  abstract predicate isParameterOf(DataFlowCallable c, int i);
+  abstract predicate isParameterOf(DataFlowCallable c, ParameterPosition pos);
 }
 
 private module ParameterNodes {
@@ -874,7 +876,9 @@ private module ParameterNodes {
         parameter
     }
 
-    override predicate isParameterOf(DataFlowCallable c, int i) { c.getParameter(i) = parameter }
+    override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
+      c.getParameter(pos.getPosition()) = parameter
+    }
 
     override DataFlowCallable getEnclosingCallableImpl() { result = parameter.getCallable() }
 
@@ -896,7 +900,9 @@ private module ParameterNodes {
     /** Gets the callable containing this implicit instance parameter. */
     Callable getCallable() { result = callable }
 
-    override predicate isParameterOf(DataFlowCallable c, int pos) { callable = c and pos = -1 }
+    override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
+      callable = c and pos.isThisParameter()
+    }
 
     override DataFlowCallable getEnclosingCallableImpl() { result = callable }
 
@@ -909,42 +915,15 @@ private module ParameterNodes {
     override string toStringImpl() { result = "this" }
   }
 
-  module ImplicitCapturedParameterNodeImpl {
-    /** An implicit entry definition for a captured variable. */
-    class SsaCapturedEntryDefinition extends Ssa::ImplicitEntryDefinition {
-      private LocalScopeVariable v;
-
-      SsaCapturedEntryDefinition() { this.getSourceVariable().getAssignable() = v }
-
-      LocalScopeVariable getVariable() { result = v }
-    }
-
-    private class CapturedVariable extends LocalScopeVariable {
-      CapturedVariable() { this = any(SsaCapturedEntryDefinition d).getVariable() }
-    }
-
-    private predicate id(CapturedVariable x, CapturedVariable y) { x = y }
-
-    private predicate idOf(CapturedVariable x, int y) = equivalenceRelation(id/2)(x, y)
+  /** An implicit entry definition for a captured variable. */
+  class SsaCapturedEntryDefinition extends Ssa::ImplicitEntryDefinition {
+    private LocalScopeVariable v;
 
-    int getId(CapturedVariable v) { idOf(v, result) }
+    SsaCapturedEntryDefinition() { this.getSourceVariable().getAssignable() = v }
 
-    // we model implicit parameters for captured variables starting from index `-2`,
-    // the order is irrelevant
-    int getParameterPosition(SsaCapturedEntryDefinition def) {
-      exists(Callable c | c = def.getCallable() |
-        def =
-          rank[-result - 1](SsaCapturedEntryDefinition def0 |
-            def0.getCallable() = c
-          |
-            def0 order by getId(def0.getSourceVariable().getAssignable())
-          )
-      )
-    }
+    LocalScopeVariable getVariable() { result = v }
   }
 
-  private import ImplicitCapturedParameterNodeImpl
-
   /**
    * The value of an implicit captured variable parameter at function entry,
    * viewed as a node in a data flow graph.
@@ -970,8 +949,8 @@ private module ParameterNodes {
     /** Gets the captured variable that this implicit parameter models. */
     LocalScopeVariable getVariable() { result = def.getVariable() }
 
-    override predicate isParameterOf(DataFlowCallable c, int i) {
-      i = getParameterPosition(def) and
+    override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
+      pos.isImplicitCapturedParameterPosition(def) and
       c = this.getEnclosingCallable()
     }
   }
@@ -982,11 +961,13 @@ import ParameterNodes
 /** A data-flow node that represents a call argument. */
 class ArgumentNode extends Node instanceof ArgumentNodeImpl {
   /** Holds if this argument occurs at the given position in the given call. */
-  final predicate argumentOf(DataFlowCall call, int pos) { super.argumentOf(call, pos) }
+  final predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
+    super.argumentOf(call, pos)
+  }
 }
 
 abstract private class ArgumentNodeImpl extends Node {
-  abstract predicate argumentOf(DataFlowCall call, int pos);
+  abstract predicate argumentOf(DataFlowCall call, ArgumentPosition pos);
 }
 
 private module ArgumentNodes {
@@ -1011,7 +992,7 @@ private module ArgumentNodes {
       this.asExpr() = any(CIL::Call call).getAnArgument()
     }
 
-    override predicate argumentOf(DataFlowCall call, int pos) {
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
       exists(ArgumentConfiguration x, Expr c, Argument arg |
         arg = this.asExpr() and
         c = call.getExpr() and
@@ -1022,7 +1003,7 @@ private module ArgumentNodes {
       exists(CIL::Call c, CIL::Expr arg |
         arg = this.asExpr() and
         c = call.getExpr() and
-        arg = c.getArgument(pos)
+        arg = c.getArgument(pos.getPosition())
       )
     }
   }
@@ -1060,10 +1041,15 @@ private module ArgumentNodes {
       )
     }
 
-    override predicate argumentOf(DataFlowCall call, int pos) {
-      exists(ImplicitCapturedParameterNode p, boolean additionalCalls |
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
+      exists(
+        ImplicitCapturedParameterNode p, boolean additionalCalls, ParameterPosition ppos,
+        SsaCapturedEntryDefinition def
+      |
         this.flowsInto(p, additionalCalls) and
-        p.isParameterOf(call.getARuntimeTarget(), pos) and
+        p.isParameterOf(call.getARuntimeTarget(), ppos) and
+        pos.isImplicitCapturedArgumentPosition(def) and
+        ppos.isImplicitCapturedParameterPosition(def) and
         call.getControlFlowNode() = cfn and
         if call instanceof TransitiveCapturedDataFlowCall
         then additionalCalls = true
@@ -1091,9 +1077,9 @@ private module ArgumentNodes {
 
     MallocNode() { this = TMallocNode(cfn) }
 
-    override predicate argumentOf(DataFlowCall call, int pos) {
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
       call = TNonDelegateCall(cfn, _) and
-      pos = -1
+      pos.isQualifier()
     }
 
     override ControlFlow::Node getControlFlowNodeImpl() { result = cfn }
@@ -1130,9 +1116,9 @@ private module ArgumentNodes {
       callCfn = any(Call c | isParamsArg(c, _, result)).getAControlFlowNode()
     }
 
-    override predicate argumentOf(DataFlowCall call, int pos) {
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
       callCfn = call.getControlFlowNode() and
-      pos = this.getParameter().getPosition()
+      pos.getPosition() = this.getParameter().getPosition()
     }
 
     override DataFlowCallable getEnclosingCallableImpl() { result = callCfn.getEnclosingCallable() }
@@ -1149,11 +1135,8 @@ private module ArgumentNodes {
   private class SummaryArgumentNode extends SummaryNode, ArgumentNodeImpl {
     SummaryArgumentNode() { FlowSummaryImpl::Private::summaryArgumentNode(_, this, _) }
 
-    override predicate argumentOf(DataFlowCall call, int pos) {
-      exists(ArgumentPosition apos |
-        FlowSummaryImpl::Private::summaryArgumentNode(call, this, apos) and
-        apos.getPosition() = pos
-      )
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
+      FlowSummaryImpl::Private::summaryArgumentNode(call, this, pos)
     }
   }
 }
@@ -1870,8 +1853,8 @@ private module PostUpdateNodes {
 
     override MallocNode getPreUpdateNode() { result.getControlFlowNode() = cfn }
 
-    override predicate argumentOf(DataFlowCall call, int pos) {
-      pos = -1 and
+    override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
+      pos.isQualifier() and
       any(ObjectOrCollectionInitializerConfiguration x)
           .hasExprPath(_, cfn, _, call.getControlFlowNode())
     }