Merge pull request #1121 from jbj/return-stack-allocated-1.20-fixes

Approved by geoffw0

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

@@ -5,3 +5,5 @@
 | test.cpp:92:2:92:12 | return ... | May return stack-allocated memory from $@. | test.cpp:89:10:89:11 | mc | mc |
 | 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: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

@@ -143,3 +143,49 @@ char *testArray5()
 
 	return arr; // GOOD
 }
+
+int *returnThreadLocal() {
+  thread_local int threadLocal;
+  return &threadLocal; // GOOD [FALSE POSITIVE]
+}
+
+int returnDereferenced() {
+  int localInt = 2;
+  int &localRef = localInt;
+  return localRef; // GOOD
+}
+
+typedef unsigned long size_t;
+void *memcpy(void *s1, const void *s2, size_t n);
+
+char *returnAfterCopy() {
+  char localBuf[] = "Data";
+  static char staticBuf[sizeof(localBuf)];
+  memcpy(staticBuf, localBuf, sizeof(staticBuf));
+  return staticBuf; // GOOD
+}
+
+void *conversionBeforeDataFlow() {
+  int myLocal;
+  void *pointerToLocal = (void *)&myLocal; // has conversion
+  return pointerToLocal; // BAD
+}
+
+void *arrayConversionBeforeDataFlow() {
+  int localArray[4];
+  int *pointerToLocal = localArray; // has conversion
+  return pointerToLocal; // BAD [NOT DETECTED]
+}
+
+int &dataFlowThroughReference() {
+  int myLocal;
+  int &refToLocal = myLocal; // has conversion
+  return refToLocal; // BAD [NOT DETECTED]
+}
+
+int *&conversionInFlow() {
+  int myLocal;
+  int *p = &myLocal;
+  int *&pRef = p; // has conversion in the middle of data flow
+  return pRef; // BAD [NOT DETECTED]
+}