Merge pull request #1 from jbj/returnstack-EscapesTree

C++: Reimplement cpp/return-stack-allocated-memory with EscapesTree and data flow

Author: geoffw0

change-notes/1.20/analysis-cpp.md

@@ -11,6 +11,7 @@
 | Use of string copy function in a condition (`cpp/string-copy-return-value-as-boolean`) | correctness | This query identifies calls to string copy functions used in conditions, where it's likely that a different function was intended to be called. |
 | Lossy function result cast (`cpp/lossy-function-result-cast`) | correctness | Finds function calls whose result type is a floating point type, which are implicitly cast to an integral type.  Newly available but not displayed by default on LGTM. |
 | Array argument size mismatch (`cpp/array-arg-size-mismatch`) | reliability | Finds function calls where the size of an array being passed is smaller than the array size of the declared parameter.  Newly displayed on LGTM. |
+| Returning stack-allocated memory (`cpp/return-stack-allocated-memory`) | reliability, external/cwe/cwe-825 | Finds functions that may return a pointer or reference to stack-allocated memory. This query existed already but has been rewritten from scratch to make the error rate low enough for use on LGTM. Displayed by default. |
 
 ## Changes to existing queries
 

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

@@ -6,53 +6,46 @@
  * @kind problem
  * @id cpp/return-stack-allocated-memory
  * @problem.severity warning
+ * @precision high
  * @tags reliability
+ *       external/cwe/cwe-825
  */
 
 import cpp
+import semmle.code.cpp.dataflow.EscapesTree
+import semmle.code.cpp.dataflow.DataFlow
 
-// an expression is possibly stack allocated if it is an aggregate literal
-// or accesses a possibly stack allocated local variables
-predicate exprMaybeStackAllocated(Expr e) {
-  e instanceof AggregateLiteral or
-  varMaybeStackAllocated(e.(VariableAccess).getTarget()) or
-  exprMayPointToStack(e.(ArrayExpr).getArrayBase())
-}
-
-// a local variable is possibly stack allocated if it is not static and
-// is initialized to/assigned a possibly stack allocated expression
-predicate varMaybeStackAllocated(LocalVariable lv) {
-  not lv.isStatic() and
-  not lv.getType() instanceof ReferenceType
+/**
+ * Holds if `n1` may flow to `n2`, ignoring flow through fields because these
+ * are currently modeled as an overapproximation that assumes all objects may
+ * alias.
+ */
+predicate conservativeDataFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+  DataFlow::localFlowStep(n1, n2) and
+  not n2.asExpr() instanceof FieldAccess
 }
 
-// an expression possibly points to the stack if it takes the address of
-// a possibly stack allocated expression, if it is a reference to a local variable
-// that possibly points to the stack, or if it is a possibly stack allocated array
-// that is converted (implicitly or explicitly) to a pointer
-predicate exprMayPointToStack(Expr e) {
-  exprMaybeStackAllocated(e.(AddressOfExpr).getAnOperand())
-  or
-  varMayPointToStack(e.(VariableAccess).getTarget())
-  or
+from LocalScopeVariable var, VariableAccess va, ReturnStmt r
+where
+  not var.isStatic() and
+  not var.getType().getUnspecifiedType() instanceof ReferenceType and
+  not r.isFromUninstantiatedTemplate(_) and
+  va = var.getAnAccess() and
   (
-    exprMaybeStackAllocated(e) and
-    e.getType() instanceof ArrayType and
-    e.getFullyConverted().getType() instanceof PointerType
+    // To check if the address escapes directly from `e` in `return e`, we need
+    // to check the fully-converted `e` in case there are implicit
+    // array-to-pointer conversions or reference conversions.
+    variableAddressEscapesTree(va, r.getExpr().getFullyConverted())
+    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.
+    exists(Expr pointerToLocal |
+      variableAddressEscapesTree(va, pointerToLocal.getFullyConverted()) and
+      conservativeDataFlowStep+(
+        DataFlow::exprNode(pointerToLocal),
+        DataFlow::exprNode(r.getExpr())
+      )
+    )
   )
-}
-
-// a local variable possibly points to the stack if it is initialized to/assigned to
-// an expression that possibly points to the stack
-predicate varMayPointToStack(LocalVariable lv) {
-  exprMayPointToStack(lv.getInitializer().getExpr())
-  or
-  exists(AssignExpr a |
-    a.getLValue().(VariableAccess).getTarget() = lv and
-    exprMayPointToStack(a.getRValue())
-  )
-}
-
-from ReturnStmt r
-where exprMayPointToStack(r.getExpr())
-select r, "May return stack-allocated memory."
+select r, "May return stack-allocated memory from $@.", va, va.toString()

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

@@ -1,6 +1,7 @@
-| test.cpp:17:2:17:12 | return ... | May return stack-allocated memory. |
-| test.cpp:25:2:25:12 | return ... | May return stack-allocated memory. |
-| test.cpp:33:2:33:12 | return ... | May return stack-allocated memory. |
-| test.cpp:92:2:92:12 | return ... | May return stack-allocated memory. |
-| test.cpp:112:2:112:12 | return ... | May return stack-allocated memory. |
-| test.cpp:119:2:119:19 | return ... | May return stack-allocated memory. |
+| test.cpp:17:2:17:12 | return ... | May return stack-allocated memory from $@. | test.cpp:17:10:17:11 | mc | mc |
+| test.cpp:25:2:25:12 | return ... | May return stack-allocated memory from $@. | test.cpp:23:18:23:19 | mc | mc |
+| test.cpp:47:2:47:11 | return ... | May return stack-allocated memory from $@. | test.cpp:47:9:47:10 | mc | mc |
+| test.cpp:54:2:54:16 | return ... | May return stack-allocated memory from $@. | test.cpp:54:11:54:12 | mc | mc |
+| 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 |

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

@@ -30,7 +30,7 @@ MyClass *test3()
 	MyClass mc;
 	MyClass *ptr = &mc;
 	ptr = nullptr;
-	return ptr; // GOOD [FALSE POSITIVE]
+	return ptr; // GOOD
 }
 
 MyClass *test4()
@@ -44,14 +44,14 @@ MyClass *test4()
 MyClass &test5()
 {
 	MyClass mc;
-	return mc; // BAD [NOT DETECTED]
+	return mc; // BAD
 }
 
 int *test6()
 {
 	MyClass mc;
 
-	return &(mc.a); // BAD [NOT DETECTED]
+	return &(mc.a); // BAD
 }
 
 MyClass test7()