C++: Require that no override of the called pure virtual function exists in any base class. This removes the false positive in the testcase. Based on the results on LGTM we have agreed to set the @precision to very-high.

Author: MathiasVP

cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.ql

@@ -6,7 +6,7 @@
  * @kind path-problem
  * @id cpp/unsafe-use-of-this
  * @problem.severity error
- * @precision high
+ * @precision very-high
  * @tags correctness
  *       language-features
  *       security
@@ -67,12 +67,11 @@ predicate getThisArgumentInitParam(
   init.getIRVariable() instanceof IRThisVariable
 }
 
-/** Holds if `instr` is a `this` pointer of type `c` used by the call instruction `call`. */
-predicate isSink(Instruction instr, CallInstruction call, Class c) {
+/** Holds if `instr` is a `this` pointer used by the call instruction `call`. */
+predicate isSink(Instruction instr, CallInstruction call) {
   exists(PureVirtualFunction func |
     call.getStaticCallTarget() = func and
     call.getThisArgument() = instr and
-    instr.getResultType().stripType() = c and
     // Weed out implicit calls to destructors of a base class
     not func instanceof Destructor
   )
@@ -95,13 +94,12 @@ predicate isSource(InitializeParameterInstruction init, string msg, Class c) {
 }
 
 /**
- * Holds if `instr` flows to a sink (which is a use of the value of `instr` as a `this` pointer
- * of type `sinkClass`).
+ * Holds if `instr` flows to a sink (which is a use of the value of `instr` as a `this` pointer).
  */
 predicate flowsToSink(Instruction instr, Instruction sink) {
   flowsFromSource(instr) and
   (
-    isSink(instr, _, _) and instr = sink
+    isSink(instr, _) and instr = sink
     or
     exists(Instruction mid |
       successor(instr, mid) and
@@ -180,17 +178,16 @@ predicate successor(Instruction instr, Instruction succ) {
 /**
  * Holds if:
  * - `source` is an initialization of a `this` pointer of type `sourceClass`, and
- * - `sink` is a use of the `this` pointer as type `sinkClass`, and
+ * - `sink` is a use of the `this` pointer, and
  * - `call` invokes a pure virtual function using `sink` as the `this` pointer, and
  * - `msg` is a string describing whether `source` is from a constructor or destructor.
  */
 predicate flows(
-  Instruction source, string msg, Class sourceClass, Instruction sink, CallInstruction call,
-  Class sinkClass
+  Instruction source, string msg, Class sourceClass, Instruction sink, CallInstruction call
 ) {
   isSource(source, msg, sourceClass) and
   flowsToSink(source, sink) and
-  isSink(sink, call, sinkClass)
+  isSink(sink, call)
 }
 
 query predicate edges(Instruction a, Instruction b) { successor(a, b) and flowsToSink(b, _) }
@@ -201,11 +198,12 @@ query predicate nodes(Instruction n, string key, string val) {
   val = n.toString()
 }
 
-from
-  Instruction source, Instruction sink, CallInstruction call, string msg, Class sourceClass,
-  Class sinkClass
+from Instruction source, Instruction sink, CallInstruction call, string msg, Class sourceClass
 where
-  flows(source, msg, sourceClass, sink, call, sinkClass) and
-  sourceClass.getABaseClass+() = sinkClass
+  flows(source, msg, sourceClass, sink, call) and
+  // Only raise an alert if there is no override of the pure virtual function in any base class.
+  not exists(Class c | c = sourceClass.getABaseClass*() |
+    c.getAMemberFunction().getAnOverriddenFunction() = call.getStaticCallTarget()
+  )
 select call.getUnconvertedResultExpression(), source, sink,
   "Call to pure virtual function during " + msg

cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/UnsafeUseOfThis.expected

@@ -59,4 +59,3 @@ nodes
 | test.cpp:25:13:25:13 | call to f | test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
 | test.cpp:35:6:35:6 | call to f | test.cpp:7:3:7:3 | InitializeParameter: B | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
 | test.cpp:35:6:35:6 | call to f | test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
-| test.cpp:48:17:48:17 | call to f | test.cpp:47:3:47:3 | InitializeParameter: F | test.cpp:48:6:48:13 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |

cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/test.cpp

@@ -45,6 +45,6 @@ struct E : public A {
 
 struct F : public E {
   F() {
-    ((A*)this)->f(); // GOOD: Will call `E::f` [FALSE POSITIVE]
+    ((A*)this)->f(); // GOOD: Will call `E::f`
   }
 };