Merge pull request #12005 from MathiasVP/flow-out-of-invalid-functions

C++: Flow out of invalid functions

Author: MathiasVP

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

@@ -360,7 +360,20 @@ class FinalParameterUse extends UseImpl, TFinalParameterUse {
   override predicate isCertain() { any() }
 
   override predicate hasIndexInBlock(IRBlock block, int index) {
-    exists(ReturnInstruction return |
+    // Ideally, this should always be a `ReturnInstruction`, but if
+    // someone forgets to write a `return` statement in a function
+    // with a non-void return type we generate an `UnreachedInstruction`.
+    // In this case we still want to generate flow out of such functions
+    // if they write to a parameter. So we pick the index of the
+    // `UnreachedInstruction` as the index of this use.
+    // Note that a function may have both a `ReturnInstruction` and an
+    // `UnreachedInstruction`. If that's the case this predicate will
+    // return multiple results. I don't think this is detrimental to
+    // performance, however.
+    exists(Instruction return |
+      return instanceof ReturnInstruction or
+      return instanceof UnreachedInstruction
+    |
       block.getInstruction(index) = return and
       return.getEnclosingFunction() = p.getFunction()
     )

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

@@ -155,8 +155,21 @@ private class FinalParameterUse extends UseImpl, TFinalParameterUse {
   override string toString() { result = p.toString() }
 
   final override predicate hasIndexInBlock(IRBlock block, int index) {
-    exists(ReturnInstruction return |
-      block.getInstruction(index + 1) = return and
+    // Ideally, this should always be a `ReturnInstruction`, but if
+    // someone forgets to write a `return` statement in a function
+    // with a non-void return type we generate an `UnreachedInstruction`.
+    // In this case we still want to generate flow out of such functions
+    // if they write to a parameter. So we pick the index of the
+    // `UnreachedInstruction` as the index of this use.
+    // Note that a function may have both a `ReturnInstruction` and an
+    // `UnreachedInstruction`. If that's the case this predicate will
+    // return multiple results. I don't think this is detrimental to
+    // performance, however.
+    exists(Instruction return |
+      return instanceof ReturnInstruction or
+      return instanceof UnreachedInstruction
+    |
+      block.getInstruction(index) = return and
       return.getEnclosingFunction() = p.getFunction()
     )
   }

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

@@ -510,7 +510,7 @@ int intOutparamSourceMissingReturn(int *p) {
 void viaOutparamMissingReturn() {
   int x = 0;
   intOutparamSourceMissingReturn(&x);
-  sink(x); // $ ast MISSING: ir
+  sink(x); // $ ast,ir
 }
 
 void uncertain_definition() {