Merge pull request #12149 from MathiasVP/fewer-flowthroughs

C++: Fix spurious flow-through

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.ql

@@ -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/lambdas.cpp

@@ -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();

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

@@ -7,16 +7,16 @@ extern int arbitrary;
 
 namespace withoutFields {
   template<typename T>
-  void assign(T &lhs, T rhs) {
+  void assign(T &lhs, T rhs) { // $ ast-def=lhs ir-def=*lhs ast-def=lhs
     lhs = rhs;
   }
 
   template<typename T>
-  void assignWrapper(T &lhs, T rhs) {
+  void assignWrapper(T &lhs, T rhs) { // $ ast-def=lhs ast-def=lhs
     assign(lhs, rhs);
   }
 
-  void notAssign(int &lhs, int rhs) {
+  void notAssign(int &lhs, int rhs) { // $ ast-def=lhs ir-def=*lhs 
     lhs = rhs;
     if (arbitrary) {
       lhs = 1;
@@ -25,14 +25,14 @@ namespace withoutFields {
     }
   }
 
-  void sourceToParam(int &out) {
+  void sourceToParam(int &out) { // $ ast-def=out ir-def=*out 
     out = source();
     if (arbitrary) {
       out = 1;
     }
   }
 
-  void sourceToParamWrapper(int &out) {
+  void sourceToParamWrapper(int &out) { // $ ast-def=out ir-def=*out 
     if (arbitrary) {
       sourceToParam(out);
     } else {
@@ -40,7 +40,7 @@ namespace withoutFields {
     }
   }
 
-  void notSource(int &out) {
+  void notSource(int &out) { // $ ast-def=out ir-def=*out
     out = source();
     if (arbitrary) {
       out = 1;
@@ -71,15 +71,15 @@ namespace withFields {
     int val;
   };
 
-  void assign(Int &lhs, int rhs) {
+  void assign(Int &lhs, int rhs) { // $ ast-def=lhs 
     lhs.val = rhs;
   }
 
-  void assignWrapper(Int &lhs, int rhs) {
+  void assignWrapper(Int &lhs, int rhs) { // $ ast-def=lhs
     assign(lhs, rhs);
   }
 
-  void notAssign(Int &lhs, int rhs) {
+  void notAssign(Int &lhs, int rhs) { // $ ast-def=lhs
     lhs.val = rhs;
     // Field flow ignores that the field is subsequently overwritten, leading
     // to false flow here.
@@ -90,14 +90,14 @@ namespace withFields {
     }
   }
 
-  void sourceToParam(Int &out) {
+  void sourceToParam(Int &out) { // $ ast-def=out
     out.val = source();
     if (arbitrary) {
       out.val = 1;
     }
   }
 
-  void sourceToParamWrapper(Int &out) {
+  void sourceToParamWrapper(Int &out) { // $ ast-def=out
     if (arbitrary) {
       sourceToParam(out);
     } else {
@@ -105,7 +105,7 @@ namespace withFields {
     }
   }
 
-  void notSource(Int &out) {
+  void notSource(Int &out) { // $ ast-def=out
     out.val = source();
     // Field flow ignores that the field is subsequently overwritten, leading
     // to false flow here.

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

@@ -63,7 +63,7 @@ namespace std {
   template<class T> T&& move(T& t) noexcept; // simplified signature
 }
 
-void identityOperations(int* source1) {
+void identityOperations(int* source1) { // $ ast-def=source1 
   const int *x1 = std::move(source1);
   int* x2 = const_cast<int*>(x1);
   int* x3 = (x2);
@@ -86,7 +86,7 @@ void trackUninitialized() {
   sink(i1); // $ ast,ir
 }
 
-void local_references(int &source1, int clean1) {
+void local_references(int &source1, int clean1) { // $ ast-def=source1 ir-def=*source1
   sink(source1); // $ ast,ir
   source1 = clean1;
   sink(source1); // clean
@@ -111,17 +111,17 @@ void local_references(int &source1, int clean1) {
   }
 }
 
-int alwaysAssignSource(int *out) {
+int alwaysAssignSource(int *out) { // $ ast-def=out ir-def=*out 
   *out = source();
   return 0;
 }
 
-int alwaysAssign0(int *out) {
+int alwaysAssign0(int *out) { // $ ast-def=out ir-def=*out
   *out = 0;
   return 0;
 }
 
-int alwaysAssignInput(int *out, int in) {
+int alwaysAssignInput(int *out, int in) { // $ ast-def=out ir-def=*out
   *out = in;
   return 0;
 }
@@ -468,7 +468,7 @@ void throughStmtExpr(int source1, int clean1) {
   sink(local); // $ ast,ir
 }
 
-void intOutparamSource(int *p) {
+void intOutparamSource(int *p) { // $ ast-def=p ir-def=*p
   *p = source();
 }
 
@@ -484,7 +484,7 @@ struct MyStruct {
   int* content; 
 };
 
-void local_field_flow_def_by_ref_steps_with_local_flow(MyStruct * s) {
+void local_field_flow_def_by_ref_steps_with_local_flow(MyStruct * s) { // $ ast-def=s 
   writes_to_content(s->content);
   int* p_content = s->content;
   sink(*p_content);
@@ -502,7 +502,7 @@ void regression_with_phi_flow(int clean1) {
   }
 }
 
-int intOutparamSourceMissingReturn(int *p) {
+int intOutparamSourceMissingReturn(int *p) { // $ ast-def=p ir-def=*p
   *p = source();
   // return deliberately omitted to test IR dataflow behavior
 }
@@ -521,23 +521,23 @@ void uncertain_definition() {
   sink(stackArray[0]); // $ ast=519:19 ir SPURIOUS: ast=517:7
 }
 
-void set_through_const_pointer(int x, const int **e)
+void set_through_const_pointer(int x, const int **e) // $ ast-def=e ir-def=**e ir-def=*e
 {
   *e = &x;
 }
 
-void test_set_through_const_pointer(int *e)
+void test_set_through_const_pointer(int *e) // $ ast-def=e
 {
   set_through_const_pointer(source(), &e);
   sink(*e); // $ ir MISSING: ast
 }
 
-void sink_then_source_1(int* p) {
+void sink_then_source_1(int* p) { // $ ast-def=p ir-def=*p
     sink(*p); // $ ir // Flow from the unitialized x to the dereference.
     *p = source();
 }
 
-void sink_then_source_2(int* p, int y) {
+void sink_then_source_2(int* p, int y) { // $ ast-def=p ir-def=*p
     sink(y); // $ SPURIOUS: ast ir
     *p = source();
 }
@@ -578,3 +578,14 @@ namespace IndirectFlowThroughGlobals {
     sink(*globalInt); // $ ir=562:17 ir=576:17 MISSING: ast=562:17 ast=576:17
   }
 }
+
+void write_to_param(int* x) { // $ ast-def=x
+  int s = source();
+  x = &s;
+}
+
+void test_write_to_param() {
+  int x = 0;
+  write_to_param(&x);
+  sink(x); // $ SPURIOUS: ast
+}