Merge pull request github#12149 from MathiasVP/fewer-flowthroughs

C++: Fix spurious flow-through

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -52,12 +52,29 @@ private newtype TIRDataFlowNode =
   TFinalParameterNode(Parameter p, int indirectionIndex) {
     exists(Ssa::FinalParameterUse use |
       use.getParameter() = p and
-      use.getIndirectionIndex() = indirectionIndex
+      use.getIndirectionIndex() = indirectionIndex and
+      parameterIsRedefined(p)
     )
   } or
   TFinalGlobalValue(Ssa::GlobalUse globalUse) or
   TInitialGlobalValue(Ssa::GlobalDef globalUse)
 
+/**
+ * Holds if the value of `*p` (or `**p`, `***p`, etc.) is redefined somewhere in the body
+ * of the enclosing function of `p`.
+ *
+ * Only parameters satisfying this predicate will generate a `FinalParameterNode` transferring
+ * flow out of the function.
+ */
+private predicate parameterIsRedefined(Parameter p) {
+  exists(Ssa::Def def |
+    def.getSourceVariable().getBaseVariable().(Ssa::BaseIRVariable).getIRVariable().getAst() = p and
+    def.getIndirectionIndex() = 0 and
+    def.getIndirection() > 1 and
+    not def.getValue().asInstruction() instanceof InitializeParameterInstruction
+  )
+}
+
 /**
  * An operand that is defined by a `FieldAddressInstruction`.
  */

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -918,6 +918,8 @@ class Def extends DefOrUse {
   Instruction getAddress() { result = this.getAddressOperand().getDef() }
 
   /**
+   * Gets the indirection index of this definition.
+   *
    * This predicate ensures that joins go from `defOrUse` to the result
    * instead of the other way around.
    */
@@ -926,6 +928,19 @@ class Def extends DefOrUse {
     pragma[only_bind_into](result) = pragma[only_bind_out](defOrUse).getIndirectionIndex()
   }
 
+  /**
+   * Gets the indirection level that this definition is writing to.
+   * For instance, `x = y` is a definition of `x` at indirection level 1 and
+   * `*x = y` is a definition of `x` at indirection level 2.
+   *
+   * This predicate ensures that joins go from `defOrUse` to the result
+   * instead of the other way around.
+   */
+  pragma[inline]
+  int getIndirection() {
+    pragma[only_bind_into](result) = pragma[only_bind_out](defOrUse).getIndirection()
+  }
+
   Node0Impl getValue() { result = defOrUse.getValue() }
 
   predicate isCertain() { defOrUse.isCertain() }

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

@@ -56,7 +56,7 @@ void bg_stackstruct(XY s1, XY s2) {
   }
 }
 
-void bg_structptr(XY *p1, XY *p2) {
+void bg_structptr(XY *p1, XY *p2) { // $ ast-def=p1 ast-def=p2
   p1->x = source();
   if (guarded(p1->x)) {
     sink(p1->x); // $ SPURIOUS: ast

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

@@ -8,7 +8,7 @@ struct twoIntFields {
   int getFirst() { return m1; }
 };
 
-void following_pointers(
+void following_pointers( // $ ast-def=sourceStruct1_ptr
     int sourceArray1[],
     int cleanArray1[],
     twoIntFields sourceStruct1,

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

@@ -107,6 +107,7 @@ postWithInFlow
 | test.cpp:552:25:552:25 | y [inner post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:562:5:562:13 | globalInt [post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:576:5:576:13 | globalInt [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:589:19:589:19 | x [inner post update] | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition
 uniqueParameterNodePosition

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

@@ -25,7 +25,7 @@ struct Bottom : Middle {
   void notSink(int x) override { }
 };
 
-void VirtualDispatch(Bottom *bottomPtr, Bottom &bottomRef) {
+void VirtualDispatch(Bottom *bottomPtr, Bottom &bottomRef) { // $ ast-def=bottomPtr ast-def=bottomRef
   Top *topPtr = bottomPtr, &topRef = bottomRef;
 
   sink(topPtr->isSource1()); // $ ir MISSING: ast
@@ -65,11 +65,11 @@ Top *allocateBottom() {
   return new Bottom();
 }
 
-void callSinkByPointer(Top *top) {
+void callSinkByPointer(Top *top) { // $ ast-def=top
   top->isSink(source()); // leads to MISSING from ast
 }
 
-void callSinkByReference(Top &top) {
+void callSinkByReference(Top &top) { // $ ast-def=top
   top.isSink(source()); // leads to MISSING from ast
 }
 
@@ -81,11 +81,11 @@ void globalVirtualDispatch() {
   x->isSink(source()); // $ MISSING: ast,ir
 }
 
-Top *identity(Top *top) {
+Top *identity(Top *top) { // $ ast-def=top
   return top;
 }
 
-void callIdentityFunctions(Top *top, Bottom *bottom) {
+void callIdentityFunctions(Top *top, Bottom *bottom) { // $ ast-def=bottom ast-def=top
   identity(bottom)->isSink(source()); // $ MISSING: ast,ir
   identity(top)->isSink(source()); // no flow
 }
@@ -120,7 +120,7 @@ namespace virtual_inheritance {
   struct Bottom : Middle {
   };
 
-  void VirtualDispatch(Bottom *bottomPtr, Bottom &bottomRef) {
+  void VirtualDispatch(Bottom *bottomPtr, Bottom &bottomRef) { // $ ast-def=bottomPtr ast-def=bottomRef
     // Because the inheritance from `Top` is virtual, the following casts go
     // directly from `Bottom` to `Top`, skipping `Middle`. That means we don't
     // get flow from a `Middle` value to the call qualifier.

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

@@ -12,7 +12,7 @@ typedef struct
 	char isTrue;
 } MyBool;
 
-void myTest_with_local_flow(MyBool *b, int pos)
+void myTest_with_local_flow(MyBool *b, int pos) // $ ast-def=b 
 {
 	MyCoords coords = {0};
 

cpp/ql/test/library-tests/dataflow/dataflow-tests/has-parameter-flow-out.expected

@@ -0,0 +1,53 @@
+import TestUtilities.InlineExpectationsTest
+import cpp
+
+module AstTest {
+  private import semmle.code.cpp.dataflow.DataFlow::DataFlow
+  private import semmle.code.cpp.dataflow.internal.DataFlowPrivate
+
+  class ASTParameterDefTest extends InlineExpectationsTest {
+    ASTParameterDefTest() { this = "ASTParameterDefTest" }
+
+    override string getARelevantTag() { result = "ast-def" }
+
+    override predicate hasActualResult(Location location, string element, string tag, string value) {
+      exists(Function f, Parameter p, RefParameterFinalValueNode n |
+        p.isNamed() and
+        n.getParameter() = p and
+        n.getFunction() = f and
+        location = f.getLocation() and
+        element = p.toString() and
+        tag = "ast-def" and
+        value = p.getName()
+      )
+    }
+  }
+}
+
+module IRTest {
+  private import semmle.code.cpp.ir.dataflow.DataFlow
+  private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
+
+  private string stars(int k) {
+    k = [0 .. max(FinalParameterNode n | | n.getIndirectionIndex())] and
+    (if k = 0 then result = "" else result = "*" + stars(k - 1))
+  }
+
+  class IRParameterDefTest extends InlineExpectationsTest {
+    IRParameterDefTest() { this = "IRParameterDefTest" }
+
+    override string getARelevantTag() { result = "ir-def" }
+
+    override predicate hasActualResult(Location location, string element, string tag, string value) {
+      exists(Function f, Parameter p, FinalParameterNode n |
+        p.isNamed() and
+        n.getParameter() = p and
+        n.getFunction() = f and
+        location = f.getLocation() and
+        element = p.toString() and
+        tag = "ir-def" and
+        value = stars(n.getIndirectionIndex()) + p.getName()
+      )
+    }
+  }
+}

cpp/ql/test/library-tests/dataflow/dataflow-tests/has-parameter-flow-out.ql

@@ -37,7 +37,7 @@ void test_lambdas()
 	};
 	d(t, u);
 
-	auto e = [](int &a, int &b, int &c) {
+	auto e = [](int &a, int &b, int &c) { // $ ast-def=a ast-def=b ast-def=c ir-def=*c
 		sink(a); // $ ast,ir
 		sink(b);
 		c = source();