C#: Rework SummarizedCallable::clearsContent/2

Author: hvitved

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

@@ -78,4 +78,21 @@ module SummaryComponentStack {
 
 class SummarizedCallable = Impl::Public::SummarizedCallable;
 
+private class SummarizedCallableDefaultClearsContent extends Impl::Public::SummarizedCallable {
+  SummarizedCallable c;
+
+  SummarizedCallableDefaultClearsContent() { this = c }
+
+  // By default, we assume that all stores into arguments are definite
+  override predicate clearsContent(int i, DataFlow::Content content) {
+    exists(SummaryComponentStack output |
+      c.propagatesFlow(_, output, _) and
+      output.drop(_) =
+        SummaryComponentStack::push(SummaryComponent::content(content),
+          SummaryComponentStack::argument(i)) and
+      not content instanceof DataFlow::ElementContent
+    )
+  }
+}
+
 class RequiredSummaryComponentStack = Impl::Public::RequiredSummaryComponentStack;

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

@@ -252,6 +252,8 @@ private class RetNodeEx extends NodeEx {
   ReturnPosition getReturnPosition() { result = getReturnPosition(this.asNode()) }
 
   ReturnKindExt getKind() { result = this.asNode().(ReturnNodeExt).getKind() }
+
+  predicate allowFlowThroughParameter() { allowFlowThroughParameterCached(this.asNode()) }
 }
 
 private predicate inBarrier(NodeEx node, Configuration config) {
@@ -725,12 +727,16 @@ private module Stage1 {
   /** Holds if flow may return from `callable`. */
   pragma[nomagic]
   private predicate returnFlowCallableNodeCand(
-    DataFlowCallable callable, ReturnKindExt kind, Configuration config
+    DataFlowCallable callable, ReturnKindExt kind, boolean allowFlowThroughParameter,
+    Configuration config
   ) {
     exists(RetNodeEx ret |
       throughFlowNodeCand(ret, config) and
       callable = ret.getEnclosingCallable() and
-      kind = ret.getKind()
+      kind = ret.getKind() and
+      if ret.allowFlowThroughParameter()
+      then allowFlowThroughParameter = true
+      else allowFlowThroughParameter = false
     )
   }
 
@@ -739,13 +745,16 @@ private module Stage1 {
    * candidate for the origin of a summary.
    */
   predicate parameterMayFlowThrough(ParamNodeEx p, DataFlowCallable c, Ap ap, Configuration config) {
-    exists(ReturnKindExt kind |
+    exists(ReturnKindExt kind, boolean allowFlowThroughParameter |
       throughFlowNodeCand(p, config) and
-      returnFlowCallableNodeCand(c, kind, config) and
+      returnFlowCallableNodeCand(c, kind, allowFlowThroughParameter, config) and
       p.getEnclosingCallable() = c and
-      exists(ap) and
-      // we don't expect a parameter to return stored in itself
-      not kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
+      (
+        if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
+        then allowFlowThroughParameter = true
+        else any()
+      ) and
+      exists(ap)
     )
   }
 
@@ -1394,8 +1403,11 @@ private module Stage2 {
       fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
       kind = ret.getKind() and
       p.getPosition() = pos and
-      // we don't expect a parameter to return stored in itself
-      not kind.(ParamUpdateReturnKind).getPosition() = pos
+      (
+        if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
+        then ret.allowFlowThroughParameter()
+        else any()
+      )
     )
   }
 
@@ -2083,8 +2095,11 @@ private module Stage3 {
       fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
       kind = ret.getKind() and
       p.getPosition() = pos and
-      // we don't expect a parameter to return stored in itself
-      not kind.(ParamUpdateReturnKind).getPosition() = pos
+      (
+        if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
+        then ret.allowFlowThroughParameter()
+        else any()
+      )
     )
   }
 
@@ -2843,8 +2858,11 @@ private module Stage4 {
       fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
       kind = ret.getKind() and
       p.getPosition() = pos and
-      // we don't expect a parameter to return stored in itself
-      not kind.(ParamUpdateReturnKind).getPosition() = pos
+      (
+        if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
+        then ret.allowFlowThroughParameter()
+        else any()
+      )
     )
   }
 
@@ -2917,6 +2935,8 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
 
   int getParameterPos() { p.isParameterOf(_, result) }
 
+  ParameterNode getParameterNode() { result = p.asNode() }
+
   override string toString() { result = p + ": " + ap }
 
   predicate hasLocationInfo(
@@ -3617,7 +3637,11 @@ private predicate paramFlowsThrough(
     ap = mid.getAp() and
     apa = ap.getApprox() and
     pos = sc.getParameterPos() and
-    not kind.(ParamUpdateReturnKind).getPosition() = pos
+    (
+      if kind.(ParamUpdateReturnKind).getPosition() = pos
+      then ret.allowFlowThroughParameter()
+      else any()
+    )
   )
 }
 

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

@@ -801,6 +801,9 @@ private module Cached {
     exists(Node n | getNodeEnclosingCallable(n) = callable | isUnreachableInCallCached(n, call))
   }
 
+  cached
+  predicate allowFlowThroughParameterCached(Node ret) { allowFlowThroughParameter(ret) }
+
   cached
   newtype TCallContext =
     TAnyCallContext() or

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

@@ -175,6 +175,7 @@ module Consistency {
 
   query predicate postWithInFlow(Node n, string msg) {
     isPostUpdateNode(n) and
+    not clearsContent(n, _) and
     simpleLocalFlowStep(_, n) and
     msg = "PostUpdateNode should not be the target of local flow."
   }

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

@@ -310,6 +310,18 @@ module LocalFlow {
     result = n.(ExplicitParameterNode).getSsaDefinition()
   }
 
+  /**
+   * Holds if there is a local use-use flow step from `nodeFrom` to `nodeTo`
+   * involving SSA definition `def`.
+   */
+  predicate localSsaFlowStepUseUse(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
+    exists(ControlFlow::Node cfnFrom, ControlFlow::Node cfnTo |
+      SsaImpl::adjacentReadPairSameVar(def, cfnFrom, cfnTo) and
+      nodeTo = TExprNode(cfnTo) and
+      nodeFrom = TExprNode(cfnFrom)
+    )
+  }
+
   /**
    * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving
    * SSA definition `def.
@@ -322,14 +334,7 @@ module LocalFlow {
     )
     or
     // Flow from read to next read
-    exists(ControlFlow::Node cfnFrom, ControlFlow::Node cfnTo |
-      SsaImpl::adjacentReadPairSameVar(def, cfnFrom, cfnTo) and
-      nodeTo = TExprNode(cfnTo)
-    |
-      nodeFrom = TExprNode(cfnFrom)
-      or
-      cfnFrom = nodeFrom.(PostUpdateNode).getPreUpdateNode().getControlFlowNode()
-    )
+    localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
     or
     // Flow into phi node
     exists(Ssa::PhiNode phi |
@@ -399,6 +404,12 @@ module LocalFlow {
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
   or
+  exists(Ssa::Definition def |
+    LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
+    not FlowSummaryImpl::Private::Steps::summaryClearsContentArg(nodeFrom, _) and
+    not LocalFlow::usesInstanceField(def)
+  )
+  or
   LocalFlow::localFlowCapturedVarStep(nodeFrom, nodeTo)
   or
   FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true)
@@ -716,6 +727,8 @@ private module Cached {
   predicate localFlowStepImpl(Node nodeFrom, Node nodeTo) {
     LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
     or
+    LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
+    or
     exists(Ssa::Definition def |
       LocalFlow::localSsaFlowStep(def, nodeFrom, nodeTo) and
       LocalFlow::usesInstanceField(def)
@@ -1691,9 +1704,6 @@ predicate clearsContent(Node n, Content c) {
   or
   fieldOrPropertyStore(_, c, _, n.(ObjectInitializerNode).getInitializer(), false)
   or
-  FlowSummaryImpl::Private::Steps::summaryStoresIntoArg(c, n) and
-  not c instanceof ElementContent
-  or
   FlowSummaryImpl::Private::Steps::summaryClearsContent(n, c)
   or
   exists(WithExpr we, ObjectInitializer oi, FieldOrProperty f |
@@ -2003,3 +2013,11 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves
     preservesValue = false
   )
 }
+
+/**
+ * Holds if flow is allowed to pass from a parameter `p`, to return
+ * node `ret`, and back out to `p`.
+ */
+predicate allowFlowThroughParameter(ReturnNodeExt ret) {
+  FlowSummaryImpl::Private::summaryAllowFlowThroughParameter(ret)
+}