C++: Recover some of the good results we lost

My recent changes to suppress FPs in `ReturnStackAllocatedMemory.ql`
caused us to lose all results where there was a `Conversion` at the
initial address escape. We cannot handle conversions in general, but
this commit restores the good results for the trivial types of
conversion that we can handle.

Author: jbj

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -22,7 +22,24 @@ import semmle.code.cpp.dataflow.DataFlow
  */
 predicate conservativeDataFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
   DataFlow::localFlowStep(n1, n2) and
-  not n2.asExpr() instanceof FieldAccess
+  not n2.asExpr() instanceof FieldAccess and
+  not hasNontrivialConversion(n2.asExpr())
+}
+
+/**
+ * Holds if `e` has a conversion that changes it from lvalue to pointer or
+ * back. As the data-flow library does not support conversions, we cannot track
+ * data flow through such expressions.
+ */
+predicate hasNontrivialConversion(Expr e) {
+  e instanceof Conversion and not
+  (
+    e instanceof Cast
+    or
+    e instanceof ParenthesisExpr
+  )
+  or
+  hasNontrivialConversion(e.getConversion())
 }
 
 from LocalScopeVariable var, VariableAccess va, ReturnStmt r
@@ -39,9 +56,10 @@ where
     or
     // The data flow library doesn't support conversions, so here we check that
     // the address escapes into some expression `pointerToLocal`, which flows
-    // in a non-trivial way (one or more steps) to a returned expression.
+    // in a one or more steps to a returned expression.
     exists(Expr pointerToLocal |
-      variableAddressEscapesTree(va, pointerToLocal) and
+      variableAddressEscapesTree(va, pointerToLocal.getFullyConverted()) and
+      not hasNontrivialConversion(pointerToLocal) and
       conservativeDataFlowStep+(
         DataFlow::exprNode(pointerToLocal),
         DataFlow::exprNode(r.getExpr())

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected

@@ -6,4 +6,4 @@
 | test.cpp:112:2:112:12 | return ... | May return stack-allocated memory from $@. | test.cpp:112:9:112:11 | arr | arr |
 | test.cpp:119:2:119:19 | return ... | May return stack-allocated memory from $@. | test.cpp:119:11:119:13 | arr | arr |
 | test.cpp:149:3:149:22 | return ... | May return stack-allocated memory from $@. | test.cpp:149:11:149:21 | threadLocal | threadLocal |
-| test.cpp:190:3:190:14 | return ... | May return stack-allocated memory from $@. | test.cpp:188:13:188:19 | myLocal | myLocal |
+| test.cpp:171:3:171:24 | return ... | May return stack-allocated memory from $@. | test.cpp:170:35:170:41 | myLocal | myLocal |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp

@@ -168,7 +168,7 @@ char *returnAfterCopy() {
 void *conversionBeforeDataFlow() {
   int myLocal;
   void *pointerToLocal = (void *)&myLocal; // has conversion
-  return pointerToLocal; // BAD [NOT DETECTED]
+  return pointerToLocal; // BAD
 }
 
 void *arrayConversionBeforeDataFlow() {
@@ -187,5 +187,5 @@ int *&conversionInFlow() {
   int myLocal;
   int *p = &myLocal;
   int *&pRef = p; // has conversion in the middle of data flow
-  return pRef; // BAD [MISLEADING ALERT MESSAGE]
+  return pRef; // BAD [NOT DETECTED]
 }