C#: Use parameter CFG nodes in SSA

Author: hvitved

csharp/ql/lib/semmle/code/csharp/Assignable.qll

@@ -271,6 +271,8 @@ module AssignableInternal {
     def = TPatternDefinition(result)
     or
     def = TAssignOperationDefinition(result)
+    or
+    def = TParameterDefaultDefinition(_, result)
   }
 
   /** A local variable declaration at the top-level of a pattern. */
@@ -308,10 +310,17 @@ module AssignableInternal {
         exists(Callable c | p = c.getAParameter() |
           c.hasBody()
           or
-          // Same as `c.(Constructor).hasInitializer()`, but avoids negative recursion warning
-          c.getAChildExpr() instanceof @constructor_init_expr
+          c.(Constructor).hasInitializer()
         )
       } or
+      TParameterDefaultDefinition(Parameter p, Expr default) {
+        exists(Callable c | p = c.getAParameter() |
+          c.hasBody()
+          or
+          c.(Constructor).hasInitializer()
+        ) and
+        default = p.getDefaultValue()
+      } or
       TAddressOfDefinition(AddressOfExpr aoe) or
       TPatternDefinition(TopLevelPatternDecl tlpd) or
       TAssignOperationDefinition(AssignOperation ao) {
@@ -349,6 +358,8 @@ module AssignableInternal {
         any(AssignableDefinitions::PatternDefinition pd | result = pd.getDeclaration().getVariable())
       or
       def = any(AssignableDefinitions::InitializerDefinition init | result = init.getAssignable())
+      or
+      def = TParameterDefaultDefinition(result, _)
     }
 
     // Not defined by dispatch in order to avoid too conservative negative recursion error
@@ -492,11 +503,6 @@ class AssignableDefinition extends TAssignableDefinition {
       exists(Ssa::ExplicitDefinition def | result = def.getAFirstReadAtNode(cfn) |
         this = def.getADefinition()
       )
-      or
-      exists(Ssa::ImplicitParameterDefinition def | result = def.getAFirstReadAtNode(cfn) |
-        this.(AssignableDefinitions::ImplicitParameterDefinition).getParameter() =
-          def.getParameter()
-      )
     )
   }
 
@@ -688,7 +694,33 @@ module AssignableDefinitions {
 
     override string toString() { result = p.toString() }
 
-    override Location getLocation() { result = this.getTarget().getLocation() }
+    override Location getLocation() { result = p.getLocation() }
+  }
+
+  /**
+   * A default value assigned to a parameter.
+   */
+  class ParameterDefaultDefinition extends AssignableDefinition, TParameterDefaultDefinition {
+    Parameter p;
+    Expr default;
+
+    ParameterDefaultDefinition() { this = TParameterDefaultDefinition(p, default) }
+
+    /** Gets the underlying parameter. */
+    Parameter getParameter() { result = p }
+
+    /** Gets the default value expression for the parameter. */
+    Expr getDefaultValue() { result = default }
+
+    override Expr getSource() { result = default }
+
+    override Expr getElement() { result = default }
+
+    override Callable getEnclosingCallable() { result = p.getCallable() }
+
+    override string toString() { result = p.toString() + " = ..." }
+
+    override Location getLocation() { result = default.getLocation() }
   }
 
   /**

csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll

@@ -205,7 +205,7 @@ private module LogicInput implements GuardsImpl::LogicInputSig {
     }
   }
 
-  class SsaParameterInit extends SsaDefinition instanceof Ssa::ImplicitParameterDefinition {
+  class SsaParameterInit extends SsaDefinition instanceof Ssa::ParameterDefinition {
     Parameter getParameter() { result = super.getParameter() }
   }
 

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

@@ -130,7 +130,7 @@ private predicate dereferenceAt(ControlFlowNode node, Ssa::Definition def, Deref
   d = def.getAReadAtNode(node)
 }
 
-private predicate isMaybeNullArgument(Ssa::ImplicitParameterDefinition def, MaybeNullExpr arg) {
+private predicate isMaybeNullArgument(Ssa::ParameterDefinition def, MaybeNullExpr arg) {
   exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
     p = def.getParameter()
   |
@@ -181,7 +181,7 @@ private predicate hasMultipleParamsArguments(Call c) {
   )
 }
 
-private predicate isNullDefaultArgument(Ssa::ImplicitParameterDefinition def, AlwaysNullExpr arg) {
+private predicate isNullDefaultArgument(Ssa::ParameterDefinition def, AlwaysNullExpr arg) {
   exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
     p = def.getParameter()
   |
@@ -337,8 +337,7 @@ class Dereference extends G::DereferenceableExpr {
         or
         p.fromSource() and
         exists(
-          Ssa::ImplicitParameterDefinition def,
-          AssignableDefinitions::ImplicitParameterDefinition pdef
+          Ssa::ParameterDefinition def, AssignableDefinitions::ImplicitParameterDefinition pdef
         |
           p = def.getParameter()
         |

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

@@ -9,7 +9,6 @@ import csharp
  */
 module Ssa {
   private import internal.SsaImpl as SsaImpl
-  private import semmle.code.csharp.internal.Location
 
   pragma[nomagic]
   private predicate assignableDefinitionLocalScopeVariable(
@@ -19,15 +18,6 @@ module Ssa {
     ad.getEnclosingCallable() = c
   }
 
-  pragma[nomagic]
-  private predicate localScopeSourceVariable(
-    SourceVariables::LocalScopeSourceVariable sv, LocalScopeVariable v, Callable c1, Callable c2
-  ) {
-    sv.getAssignable() = v and
-    sv.getEnclosingCallable() = c1 and
-    v.getCallable() = c2
-  }
-
   /**
    * A variable that can be SSA converted.
    *
@@ -54,7 +44,7 @@ module Ssa {
       not exists(result.getTargetAccess()) and
       exists(LocalScopeVariable v, Callable c |
         assignableDefinitionLocalScopeVariable(result, v, c) and
-        localScopeSourceVariable(this, v, c, _)
+        SsaImpl::localScopeSourceVariable(this, v, c, _)
       )
     }
 
@@ -482,8 +472,8 @@ module Ssa {
 
   /**
    * An SSA definition representing the implicit initialization of a variable
-   * at the beginning of a callable. Either a parameter, a local scope variable
-   * captured by the callable, or a field or property accessed inside the callable.
+   * at the beginning of a callable. Either a local scope variable captured by
+   * the callable or a field or property accessed inside the callable.
    */
   class ImplicitEntryDefinition extends ImplicitDefinition {
     ImplicitEntryDefinition() {
@@ -507,51 +497,37 @@ module Ssa {
     override Location getLocation() { result = this.getCallable().getLocation() }
   }
 
-  private module NearestLocationInput implements NearestLocationInputSig {
-    class C = ImplicitParameterDefinition;
+  deprecated class ImplicitParameterDefinition = ParameterDefinition;
 
-    predicate relevantLocations(ImplicitParameterDefinition def, Location l1, Location l2) {
-      not def.getBasicBlock() instanceof EntryBasicBlock and
-      l1 = def.getParameter().getALocation() and
-      l2 = def.getBasicBlock().getLocation()
-    }
-  }
+  final class ParameterDefinition = SsaImpl::ParameterDefinitionImpl;
 
-  pragma[nomagic]
-  private predicate implicitEntryDef(ImplicitEntryDefinition def, SourceVariable v, Callable c) {
-    v = def.getSourceVariable() and
-    c = def.getCallable()
+  private class ExplicitParameterDefinition extends ExplicitDefinition,
+    SsaImpl::ParameterDefinitionImpl
+  {
+    private Parameter p;
+    override AssignableDefinitions::ImplicitParameterDefinition ad;
+
+    ExplicitParameterDefinition() { p = ad.getParameter() }
+
+    override Parameter getParameter() { result = p }
+
+    override string toString() { result = SsaImpl::ParameterDefinitionImpl.super.toString() }
   }
 
   /**
-   * An SSA definition representing the implicit initialization of a parameter
-   * at the beginning of a callable.
+   * An SSA definition representing the default value of a parameter.
    */
-  class ImplicitParameterDefinition extends ImplicitEntryDefinition {
+  class ParameterDefaultDefinition extends ExplicitDefinition {
     private Parameter p;
+    override AssignableDefinitions::ParameterDefaultDefinition ad;
 
-    ImplicitParameterDefinition() {
-      exists(SourceVariable sv, Callable c |
-        implicitEntryDef(this, sv, c) and
-        localScopeSourceVariable(sv, p, _, c)
-      )
-    }
+    ParameterDefaultDefinition() { p = ad.getParameter() }
 
     /** Gets the parameter that this entry definition represents. */
     Parameter getParameter() { result = p }
 
-    override Element getElement() { result = this.getParameter() }
-
     override string toString() {
-      result = "SSA param(" + pragma[only_bind_out](this.getParameter()) + ")"
-    }
-
-    override Location getLocation() {
-      not NearestLocation<NearestLocationInput>::nearestLocation(this, _, _) and
-      result = p.getLocation()
-      or
-      // multi-bodied method: use matching parameter location
-      NearestLocation<NearestLocationInput>::nearestLocation(this, result, _)
+      result = "SSA param_default(" + pragma[only_bind_out](this.getParameter()) + ")"
     }
   }
 

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

@@ -1283,7 +1283,7 @@ private module NearestLocationInputParamAfterCallable implements NearestLocation
 
 private module ParameterNodes {
   pragma[nomagic]
-  private predicate ssaParamDef(Ssa::ImplicitParameterDefinition ssaDef, Parameter p, Location l) {
+  private predicate ssaParamDef(Ssa::ParameterDefinition ssaDef, Parameter p, Location l) {
     p = ssaDef.getParameter() and
     l = ssaDef.getLocation()
   }
@@ -1338,7 +1338,7 @@ private module ParameterNodes {
     }
 
     /** Gets the SSA definition corresponding to this parameter, if any. */
-    Ssa::ImplicitParameterDefinition getSsaDefinition() {
+    Ssa::ParameterDefinition getSsaDefinition() {
       exists(Parameter p, Location l |
         l = this.getParameterLocation(p) and
         ssaParamDef(result, p, l)

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

@@ -126,7 +126,6 @@ private module SourceVariableImpl {
    */
   predicate variableDefinition(BasicBlock bb, int i, Ssa::SourceVariable v, AssignableDefinition ad) {
     ad = v.getADefinition() and
-    ad.getExpr().getControlFlowNode() = bb.getNode(i) and
     // In cases like `(x, x) = (0, 1)`, we discard the first (dead) definition of `x`
     not exists(TupleAssignmentDefinition first, TupleAssignmentDefinition second | first = ad |
       second.getAssignment() = first.getAssignment() and
@@ -136,7 +135,13 @@ private module SourceVariableImpl {
     // In cases like `M(out x, out x)`, there is no inherent evaluation order, so we
     // collapse the two definitions of `x`, using the first access as the representative,
     // and expose both definitions in `ExplicitDefinition.getADefinition()`
-    not ad = getASameOutRefDefAfter(v, _)
+    not ad = getASameOutRefDefAfter(v, _) and
+    (
+      ad.getExpr().getControlFlowNode() = bb.getNode(i)
+      or
+      ad.(AssignableDefinitions::ImplicitParameterDefinition).getParameter().getControlFlowNode() =
+        bb.getNode(i)
+    )
   }
 
   /**
@@ -754,9 +759,6 @@ private module Cached {
   cached
   predicate implicitEntryDefinition(BasicBlock bb, Ssa::SourceVariable v) {
     exists(Callable c |
-      // In case `c` has multiple bodies, we want each body to get its own implicit
-      // entry definition, so we use the basic block containing the body instead of
-      // the entry block.
       bb = c.getBody().getBasicBlock() and
       c = v.getEnclosingCallable()
     |
@@ -770,6 +772,10 @@ private module Cached {
       // Each tracked field and property has an implicit entry definition
       v instanceof PlainFieldOrPropSourceVariable
       or
+      // In case `c` has multiple bodies, we want each body to get its own implicit
+      // entry definition, so we use the basic block containing the body instead of
+      // the entry block.
+      strictcount(c.getBody()) > 1 and
       v.getAssignable() instanceof Parameter
     )
   }
@@ -961,7 +967,7 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
   predicate ssaDefHasSource(WriteDefinition def) {
     // exclude flow directly from RHS to SSA definition, as we instead want to
     // go from RHS to matching assignable definition, and from there to SSA definition
-    def instanceof Ssa::ImplicitParameterDefinition
+    def instanceof Ssa::ParameterDefinition
   }
 
   /**
@@ -993,3 +999,64 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
 }
 
 private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;
+
+private import semmle.code.csharp.internal.Location
+private import semmle.code.csharp.dataflow.SSA
+
+private module NearestLocationInput implements NearestLocationInputSig {
+  class C = MultiBodyParameterDefinition;
+
+  predicate relevantLocations(MultiBodyParameterDefinition def, Location l1, Location l2) {
+    l1 = def.getParameter().getALocation() and
+    l2 = def.getBasicBlock().getLocation()
+  }
+}
+
+pragma[nomagic]
+private predicate implicitEntryDef(
+  Ssa::ImplicitEntryDefinition def, Ssa::SourceVariable v, Callable c
+) {
+  v = def.getSourceVariable() and
+  c = def.getCallable()
+}
+
+pragma[nomagic]
+predicate localScopeSourceVariable(
+  Ssa::SourceVariables::LocalScopeSourceVariable sv, LocalScopeVariable v, Callable c1, Callable c2
+) {
+  sv.getAssignable() = v and
+  sv.getEnclosingCallable() = c1 and
+  v.getCallable() = c2
+}
+
+/**
+ * An SSA definition representing the implicit initialization of a parameter
+ * at the beginning of a callable.
+ */
+abstract class ParameterDefinitionImpl extends Ssa::Definition {
+  /** Gets the parameter that this definition represents. */
+  abstract Parameter getParameter();
+
+  override string toString() {
+    result = "SSA param(" + pragma[only_bind_out](this.getParameter()) + ")"
+  }
+}
+
+class MultiBodyParameterDefinition extends ParameterDefinitionImpl, Ssa::ImplicitEntryDefinition {
+  private Parameter p;
+
+  MultiBodyParameterDefinition() {
+    exists(Ssa::SourceVariable sv, Callable c |
+      implicitEntryDef(this, sv, c) and
+      localScopeSourceVariable(sv, p, _, c)
+    )
+  }
+
+  override Parameter getParameter() { result = p }
+
+  override string toString() { result = ParameterDefinitionImpl.super.toString() }
+
+  override Location getLocation() {
+    NearestLocation<NearestLocationInput>::nearestLocation(this, result, _)
+  }
+}

csharp/ql/test/library-tests/csharp7/DefUse.ql

@@ -6,7 +6,7 @@ where
   (
     ult.(Ssa::ExplicitDefinition).getADefinition() = def
     or
-    ult.(Ssa::ImplicitParameterDefinition).getParameter() =
+    ult.(Ssa::ParameterDefinition).getParameter() =
       def.(AssignableDefinitions::ImplicitParameterDefinition).getParameter()
   ) and
   read = ssaDef.getARead()

csharp/ql/test/library-tests/dataflow/defuse/parameterUseEquivalence.ql

@@ -24,8 +24,7 @@ private LocalScopeVariableRead getAReachableUncertainRead(
   AssignableDefinitions::ImplicitParameterDefinition p
 ) {
   exists(Ssa::Definition ssaDef |
-    p.getParameter() =
-      ssaDef.getAnUltimateDefinition().(Ssa::ImplicitParameterDefinition).getParameter()
+    p.getParameter() = ssaDef.getAnUltimateDefinition().(Ssa::ParameterDefinition).getParameter()
   |
     result = ssaDef.getARead()
   )

csharp/ql/test/library-tests/dataflow/local/DataFlow.expected

@@ -5,6 +5,7 @@
 | LocalDataFlow.cs:315:15:315:20 | access to local variable sink73 |
 | LocalDataFlow.cs:316:15:316:20 | access to local variable sink74 |
 | LocalDataFlow.cs:342:15:342:21 | access to parameter tainted |
+| LocalDataFlow.cs:387:15:387:15 | access to parameter s |
 | SSA.cs:9:15:9:22 | access to local variable ssaSink0 |
 | SSA.cs:25:15:25:22 | access to local variable ssaSink1 |
 | SSA.cs:43:15:43:22 | access to local variable ssaSink2 |