Dataflow: Review fixes.

Author: aschackmull

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

@@ -546,7 +546,11 @@ module LocalFlow {
     )
     or
     hasNodePath(any(LocalExprStepConfiguration x), node1, node2) and
-    (node2 instanceof SsaDefinitionExtNode or node2.asExpr() instanceof Cast)
+    (
+      node2 instanceof SsaDefinitionExtNode or
+      node2.asExpr() instanceof Cast or
+      node2.asExpr() instanceof AssignExpr
+    )
     or
     exists(SsaImpl::Definition def |
       def = getSsaDefinitionExt(node1) and

shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

@@ -1316,7 +1316,7 @@ module MakeImpl<InputSig Lang> {
           )
           or
           // flow into a callable
-          fwdFlowIn(_, node, state, _, cc, _, _, _, t, ap, apa) and
+          fwdFlowIn(_, _, node, state, _, cc, _, _, _, t, ap, apa, _) and
           if PrevStage::parameterMayFlowThrough(node, apa)
           then (
             summaryCtx = TParamNodeSome(node.asNode()) and
@@ -1476,15 +1476,14 @@ module MakeImpl<InputSig Lang> {
 
         pragma[nomagic]
         private predicate fwdFlowIn(
-          DataFlowCall call, ParamNodeEx p, FlowState state, Cc outercc, CcCall innercc,
-          ParamNodeOption summaryCtx, TypOption argT, ApOption argAp, Typ t, Ap ap, ApApprox apa
+          DataFlowCall call, DataFlowCallable inner, ParamNodeEx p, FlowState state, Cc outercc,
+          CcCall innercc, ParamNodeOption summaryCtx, TypOption argT, ApOption argAp, Typ t, Ap ap,
+          ApApprox apa, boolean cc
         ) {
-          exists(DataFlowCallable inner, boolean cc |
-            fwdFlowInCand(call, inner, p, state, outercc, summaryCtx, argT, argAp, t, ap, apa) and
-            FwdTypeFlow::typeFlowValidEdgeIn(call, inner, cc) and
-            innercc = getCallContextCall(call, inner) and
-            if outercc instanceof CcCall then cc = true else cc = false
-          )
+          fwdFlowInCand(call, inner, p, state, outercc, summaryCtx, argT, argAp, t, ap, apa) and
+          FwdTypeFlow::typeFlowValidEdgeIn(call, inner, cc) and
+          innercc = getCallContextCall(call, inner) and
+          if outercc instanceof CcCall then cc = true else cc = false
         }
 
         bindingset[ctx, result]
@@ -1535,6 +1534,7 @@ module MakeImpl<InputSig Lang> {
           )
         }
 
+        pragma[nomagic]
         private predicate fwdFlowOut(
           DataFlowCall call, DataFlowCallable inner, NodeEx out, FlowState state, CcNoCall outercc,
           ParamNodeOption summaryCtx, TypOption argT, ApOption argAp, Typ t, Ap ap, ApApprox apa
@@ -1557,11 +1557,9 @@ module MakeImpl<InputSig Lang> {
 
           pragma[nomagic]
           predicate dataFlowTakenCallEdgeIn(DataFlowCall call, DataFlowCallable c, boolean cc) {
-            exists(ParamNodeEx p, Cc outercc, FlowState state, Cc innercc, Typ t, Ap ap |
-              fwdFlowIn(call, p, state, outercc, innercc, _, _, _, t, ap, _) and
-              fwdFlow1(p, state, innercc, _, _, _, t, _, ap, _) and
-              c = p.getEnclosingCallable() and
-              if outercc instanceof CcCall then cc = true else cc = false
+            exists(ParamNodeEx p, FlowState state, Cc innercc, Typ t, Ap ap |
+              fwdFlowIn(call, c, p, state, _, innercc, _, _, _, t, ap, _, cc) and
+              fwdFlow1(p, state, innercc, _, _, _, t, _, ap, _)
             )
           }
 
@@ -1647,8 +1645,8 @@ module MakeImpl<InputSig Lang> {
           ApOption argAp, ParamNodeEx p, Typ t, Ap ap
         ) {
           exists(ApApprox apa |
-            fwdFlowIn(call, pragma[only_bind_into](p), _, cc, innerCc, summaryCtx, argT, argAp, t,
-              ap, pragma[only_bind_into](apa)) and
+            fwdFlowIn(call, _, pragma[only_bind_into](p), _, cc, innerCc, summaryCtx, argT, argAp,
+              t, ap, pragma[only_bind_into](apa), _) and
             PrevStage::parameterMayFlowThrough(p, apa) and
             PrevStage::callMayFlowThroughRev(call)
           )
@@ -1802,7 +1800,7 @@ module MakeImpl<InputSig Lang> {
           or
           // flow out of a callable
           exists(ReturnPosition pos |
-            revFlowOut(_, node, pos, state, _, _, ap) and
+            revFlowOut(_, node, pos, state, _, _, _, ap) and
             if returnFlowsThrough(node, pos, state, _, _, _, _, ap)
             then (
               returnCtx = TReturnCtxMaybeFlowThrough(pos) and
@@ -1867,10 +1865,9 @@ module MakeImpl<InputSig Lang> {
 
           pragma[nomagic]
           predicate dataFlowTakenCallEdgeIn(DataFlowCall call, DataFlowCallable c, boolean cc) {
-            exists(RetNodeEx ret, ReturnCtx returnCtx |
-              revFlowOut(call, ret, _, _, returnCtx, _, _) and
-              c = ret.getEnclosingCallable() and
-              if returnCtx instanceof TReturnCtxNone then cc = false else cc = true
+            exists(RetNodeEx ret |
+              revFlowOut(call, ret, _, _, _, cc, _, _) and
+              c = ret.getEnclosingCallable()
             )
           }
 
@@ -1925,9 +1922,9 @@ module MakeImpl<InputSig Lang> {
         pragma[nomagic]
         private predicate revFlowOut(
           DataFlowCall call, RetNodeEx ret, ReturnPosition pos, FlowState state,
-          ReturnCtx returnCtx, ApOption returnAp, Ap ap
+          ReturnCtx returnCtx, boolean cc, ApOption returnAp, Ap ap
         ) {
-          exists(NodeEx out, boolean cc |
+          exists(NodeEx out |
             revFlow(out, state, returnCtx, returnAp, ap) and
             flowOutOfCallApValid(call, ret, pos, out, ap, cc) and
             if returnCtx instanceof TReturnCtxNone then cc = false else cc = true
@@ -1963,7 +1960,7 @@ module MakeImpl<InputSig Lang> {
           DataFlowCall call, ReturnCtx returnCtx, ApOption returnAp, ReturnPosition pos, Ap ap
         ) {
           exists(RetNodeEx ret, FlowState state, CcCall ccc |
-            revFlowOut(call, ret, pos, state, returnCtx, returnAp, ap) and
+            revFlowOut(call, ret, pos, state, returnCtx, _, returnAp, ap) and
             returnFlowsThrough(ret, pos, state, ccc, _, _, _, ap) and
             matchesCall(ccc, call)
           )

shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll

@@ -1002,7 +1002,7 @@ module MakeImplCommon<InputSig Lang> {
    * flow between parameter and argument nodes in the cases where it is possible
    * for a type to first be weakened and then strengthened again. When the
    * stronger types at the end-points of such a type flow path are incompatible,
-   * the call relevant call edges can be excluded as impossible.
+   * the relevant call edges can be excluded as impossible.
    *
    * The predicates `relevantCallEdgeIn` and `relevantCallEdgeOut` give the
    * graph to be explored prior to the recursion, and the other three predicates
@@ -1021,7 +1021,7 @@ module MakeImplCommon<InputSig Lang> {
     }
 
     /**
-     * Holds if a sequence calls may propagates the value of `p` to some
+     * Holds if a sequence of calls may propagate the value of `p` to some
      * argument-to-parameter call edge that strengthens the static type.
      */
     pragma[nomagic]
@@ -1033,7 +1033,7 @@ module MakeImplCommon<InputSig Lang> {
     }
 
     /**
-     * Holds if a sequence calls may propagates the value of `arg` to some
+     * Holds if a sequence of calls may propagate the value of `arg` to some
      * argument-to-parameter call edge that strengthens the static type.
      */
     pragma[nomagic]
@@ -1048,6 +1048,9 @@ module MakeImplCommon<InputSig Lang> {
         )
         or
         exists(ParamNode p, DataFlowType at, DataFlowType pt |
+          // A call edge may implicitly strengthen a type by ensuring that a
+          // specific argument node was reached if the type of that argument was
+          // strengthened via a cast.
           at = getNodeType(arg) and
           pt = getNodeType(p) and
           paramMustFlow(p, arg) and
@@ -1072,6 +1075,10 @@ module MakeImplCommon<InputSig Lang> {
         DataFlowCall call1, DataFlowCallable c1, ArgNode argOut, DataFlowCall call2,
         DataFlowCallable c2, ArgNode argIn
       |
+        // Data flow may exit `call1` and enter `call2`. If a stronger type is
+        // known for `argOut`, `argIn` may reach a strengthening, and both are
+        // determined by the same parameter `p` so we know they're equal, then
+        // we should track those nodes.
         trackedParamTypeCand(p) and
         callEdge(call1, c1, argOut, _) and
         Input::relevantCallEdgeOut(call1, c1) and
@@ -1152,6 +1159,15 @@ module MakeImplCommon<InputSig Lang> {
       paramMustFlow(_, arg)
     }
 
+    /**
+     * Gets the strongest of the two types `t1` and `t2`. If neither type is
+     * stronger then compatibility is checked and `t1` is returned.
+     */
+    bindingset[t1, t2]
+    DataFlowType getStrongestType(DataFlowType t1, DataFlowType t2) {
+      if typeStrongerThan(t2, t1) then result = t2 else (compatibleTypes(t1, t2) and result = t1)
+    }
+
     /**
      * Holds if `t` is a possible type for an argument reaching the tracked
      * parameter `p` through an in-going edge in the current data flow stage.
@@ -1179,15 +1195,15 @@ module MakeImplCommon<InputSig Lang> {
         cc = true and
         typeFlowParamTypeCand(p, t1) and
         nodeDataFlowType(p, t2) and
-        if typeStrongerThan(t2, t1) then t = t2 else (compatibleTypes(t1, t2) and t = t1)
+        t = getStrongestType(t1, t2)
       )
       or
       exists(ArgNode arg, DataFlowType t1, DataFlowType t2 |
         cc = false and
         typeFlowArgTypeFromReturn(arg, t1) and
         paramMustFlow(p, arg) and
         nodeDataFlowType(p, t2) and
-        if typeStrongerThan(t2, t1) then t = t2 else (compatibleTypes(t1, t2) and t = t1)
+        t = getStrongestType(t1, t2)
       )
       or
       exists(DataFlowCall call |
@@ -1208,7 +1224,7 @@ module MakeImplCommon<InputSig Lang> {
         dataFlowTakenCallEdgeOut(_, _, arg, p) and
         (if trackedParamType(p) then typeFlowParamType(p, t1, false) else nodeDataFlowType(p, t1)) and
         nodeDataFlowType(arg, t2) and
-        if typeStrongerThan(t2, t1) then t = t2 else (compatibleTypes(t1, t2) and t = t1)
+        t = getStrongestType(t1, t2)
       )
     }
 
@@ -1224,7 +1240,7 @@ module MakeImplCommon<InputSig Lang> {
           paramMustFlow(p, arg) and
           typeFlowParamType(p, t1, cc) and
           nodeDataFlowType(arg, t2) and
-          if typeStrongerThan(t2, t1) then t = t2 else (compatibleTypes(t1, t2) and t = t1)
+          t = getStrongestType(t1, t2)
         )
         or
         cc = [true, false] and