C++: BlockVar value stops at def by ref (partial)

cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll

@@ -51,6 +51,12 @@ cached class FlowVar extends TFlowVar {
    */
   cached abstract predicate definedByExpr(Expr e, ControlFlowNode node);
 
+  /**
+   * Holds if this `FlowVar` corresponds to the data written by a call that
+   * passes a variable as argument `arg`.
+   */
+  cached abstract predicate definedByReference(Expr arg);
+
   /**
    * Holds if this `FlowVar` corresponds to the initial value of `v`. The following
    * is an exhaustive list of cases where this may happen.
@@ -137,6 +143,8 @@ module FlowVar_internal {
         or
         assignmentLikeOperation(sbb, v, _)
         or
+        blockVarDefinedByReference(sbb, v, _)
+        or
         blockVarDefinedByVariable(sbb, v)
       )
     }
@@ -174,6 +182,11 @@ module FlowVar_internal {
        else node = def.getDefinition())
     }
 
+    override predicate definedByReference(Expr arg) {
+      definitionByReference(v.getAnAccess(), arg) and
+      arg = def.getDefinition()
+    }
+
     override predicate definedByInitialValue(LocalScopeVariable param) {
       def.definedByParameter(param) and
       param = v
@@ -191,6 +204,8 @@ module FlowVar_internal {
       this.definedByExpr(_, _)
       or
       this.definedByInitialValue(_)
+      or
+      this.definedByReference(_)
     }
 
     /**
@@ -221,7 +236,17 @@ module FlowVar_internal {
     BlockVar() { this = TBlockVar(sbb, v) }
 
     override VariableAccess getAnAccess() {
-      variableAccessInSBB(v, getAReachedBlockVarSBB(this), result)
+      exists(SubBasicBlock reached |
+        reached = getAReachedBlockVarSBB(this)
+      |
+        variableAccessInSBB(v, reached, result)
+        or
+        // Allow flow into a `VariableAccess` that is used as definition by
+        // reference. This flow is blocked by `getAReachedBlockVarSBB` because
+        // flow should not propagate past that.
+        result = reached.getASuccessor().(VariableAccess) and
+        blockVarDefinedByReference(result, v, _)
+      )
     }
 
     override predicate definedByInitialValue(LocalScopeVariable lsv) {
@@ -237,6 +262,10 @@ module FlowVar_internal {
       node = sbb.getANode()
     }
 
+    override predicate definedByReference(Expr arg) {
+      blockVarDefinedByReference(sbb, v, arg)
+    }
+
     override string toString() {
       exists(Expr e |
         this.definedByExpr(e, _) and
@@ -246,9 +275,15 @@ module FlowVar_internal {
       this.definedByInitialValue(_) and
       result = "initial value of "+ v
       or
+      exists(Expr arg |
+        this.definedByReference(arg) and
+        result = "ref def: "+ arg
+      )
+      or
       // impossible case
       not this.definedByExpr(_, _) and
       not this.definedByInitialValue(_) and
+      not this.definedByReference(_) and
       result = "undefined "+ v
     }
 
@@ -373,7 +408,8 @@ module FlowVar_internal {
       mid = getAReachedBlockVarSBB(start) and
       result = mid.getASuccessor() and
       not skipLoop(mid, result, sbbDef, v) and
-      not assignmentLikeOperation(result, v, _)
+      not assignmentLikeOperation(result, v, _) and
+      not blockVarDefinedByReference(result, v, _)
     )
   }
 
@@ -481,6 +517,9 @@ module FlowVar_internal {
    */
   predicate overwrite(VariableAccess va, ControlFlowNode node) {
     va = node.(AssignExpr).getLValue()
+    or
+    va = node and
+    definitionByReference(node, _)
   }
 
   /**
@@ -515,6 +554,11 @@ module FlowVar_internal {
     )
   }
 
+  predicate blockVarDefinedByReference(ControlFlowNode node, Variable v, Expr argument) {
+    node = v.getAnAccess() and
+    definitionByReference(node, argument)
+  }
+
   /**
    * Holds if `v` is initialized by `init` to have value `assignedExpr`.
    */
@@ -534,8 +578,11 @@ module FlowVar_internal {
   class DataFlowSubBasicBlockCutNode extends SubBasicBlockCutNode {
     DataFlowSubBasicBlockCutNode() {
       exists(Variable v |
-        not fullySupportedSsaVariable(v) and
+        not fullySupportedSsaVariable(v)
+      |
         assignmentLikeOperation(this, v, _)
+        or
+        blockVarDefinedByReference(this, v, _)
         // It is not necessary to cut the basic blocks at `Initializer` nodes
         // because the affected variable can have no _other_ value before its
         // initializer. It is not necessary to cut basic blocks at procedure

cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected

@@ -34,6 +34,3 @@
 | test.cpp:436:66:436:66 | b | test.cpp:441:7:441:7 | b |
 | test.cpp:437:12:437:13 | 0 | test.cpp:438:19:438:21 | tmp |
 | test.cpp:437:12:437:13 | 0 | test.cpp:439:11:439:13 | tmp |
-| test.cpp:437:12:437:13 | 0 | test.cpp:439:33:439:35 | tmp |
-| test.cpp:437:12:437:13 | 0 | test.cpp:440:8:440:10 | tmp |
-| test.cpp:437:12:437:13 | 0 | test.cpp:442:10:442:12 | tmp |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp

@@ -453,5 +453,5 @@ void cleanedByMemcpy_blockvar(int clean1) {
   int tmp;
   int *capture = &tmp;
   memcpy(&tmp, &clean1, sizeof tmp);
-  sink(tmp); // clean (FALSE POSITIVE)
+  sink(tmp); // clean
 }

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected

@@ -27,8 +27,6 @@
 | test.cpp:366:7:366:7 | x | test.cpp:362:4:362:9 | call to source |
 | test.cpp:397:10:397:18 | globalVar | test.cpp:395:17:395:22 | call to source |
 | test.cpp:423:10:423:14 | field | test.cpp:421:13:421:18 | call to source |
-| test.cpp:449:8:449:10 | tmp | test.cpp:447:7:447:9 | tmp |
-| test.cpp:456:8:456:10 | tmp | test.cpp:453:7:453:9 | tmp |
 | true_upon_entry.cpp:21:8:21:8 | x | true_upon_entry.cpp:17:11:17:16 | call to source |
 | true_upon_entry.cpp:29:8:29:8 | x | true_upon_entry.cpp:27:9:27:14 | call to source |
 | true_upon_entry.cpp:39:8:39:8 | x | true_upon_entry.cpp:33:11:33:16 | call to source |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected

@@ -9,8 +9,6 @@
 | test.cpp:136:27:136:32 | test.cpp:140:22:140:23 | AST only |
 | test.cpp:395:17:395:22 | test.cpp:397:10:397:18 | AST only |
 | test.cpp:421:13:421:18 | test.cpp:423:10:423:14 | AST only |
-| test.cpp:447:7:447:9 | test.cpp:449:8:449:10 | AST only |
-| test.cpp:453:7:453:9 | test.cpp:456:8:456:10 | AST only |
 | true_upon_entry.cpp:9:11:9:16 | true_upon_entry.cpp:13:8:13:8 | IR only |
 | true_upon_entry.cpp:62:11:62:16 | true_upon_entry.cpp:66:8:66:8 | IR only |
 | true_upon_entry.cpp:98:11:98:16 | true_upon_entry.cpp:105:8:105:8 | IR only |

cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected

@@ -1,8 +1,4 @@
 | test.cpp:75:7:75:8 | u1 | test.cpp:76:8:76:9 | u1 |
 | test.cpp:83:7:83:8 | u2 | test.cpp:84:13:84:14 | u2 |
 | test.cpp:83:7:83:8 | u2 | test.cpp:85:8:85:9 | u2 |
-| test.cpp:447:7:447:9 | tmp | test.cpp:448:11:448:13 | tmp |
-| test.cpp:447:7:447:9 | tmp | test.cpp:449:8:449:10 | tmp |
 | test.cpp:453:7:453:9 | tmp | test.cpp:454:19:454:21 | tmp |
-| test.cpp:453:7:453:9 | tmp | test.cpp:455:11:455:13 | tmp |
-| test.cpp:453:7:453:9 | tmp | test.cpp:456:8:456:10 | tmp |