Merge pull request #846 from geoffw0/returnstack

CPP: Improve  ReturnStackAllocatedMemory.ql

change-notes/1.20/analysis-cpp.md

@@ -11,12 +11,14 @@
 | 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
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Array argument size mismatch (`cpp/array-arg-size-mismatch`) | Fewer false positives | An exception has been added to this query for variable sized arrays. |
+| Returning stack-allocated memory (`cpp/return-stack-allocated-memory`) | More correct results | Many more stack allocated expressions are now recognized. |
 | Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positives | Pointer arithmetic on `char * const` expressions (and other variations of `char *`) are now correctly excluded from the results. |
 | Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positives | False positives involving types that are not uniquely named in the snapshot have been fixed. |
 | Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | Calls to `fread` are now examined by this query. |

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

@@ -6,45 +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 a reference to a possibly stack allocated local variables
-predicate exprMaybeStackAllocated(Expr e) {
-     e instanceof AggregateLiteral
-  or varMaybeStackAllocated(e.(VariableAccess).getTarget())
+/**
+ * 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
 }
 
-// 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
-  (   lv.getType().getUnderlyingType() instanceof ArrayType
-   or exprMaybeStackAllocated(lv.getInitializer().getExpr())
-   or exists(AssignExpr a | a.getLValue().(VariableAccess).getTarget() = lv and
-                            exprMaybeStackAllocated(a.getRValue())))
-}
-
-// 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) {
-     e instanceof AddressOfExpr and exprMaybeStackAllocated(e.(AddressOfExpr).getAnOperand())
-  or varMayPointToStack(e.(VariableAccess).getTarget())
-  or exprMaybeStackAllocated(e) and e.getType() instanceof ArrayType and e.getFullyConverted().getType() instanceof PointerType
-}
-
-// 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."
+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
+  (
+    // 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())
+      )
+    )
+  )
+select r, "May return stack-allocated memory from $@.", va, va.toString()

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

@@ -0,0 +1,7 @@
+| 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/ReturnStackAllocatedMemory.qlref

@@ -0,0 +1 @@
+Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

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

@@ -0,0 +1,145 @@
+
+class MyClass
+{
+public:
+	int a, b;
+};
+
+MyClass makeMyClass()
+{
+	return { 0, 0 }; // GOOD
+}
+
+MyClass *test1()
+{
+	MyClass mc;
+
+	return &mc; // BAD
+}
+
+MyClass *test2()
+{
+	MyClass mc;
+	MyClass *ptr = &mc;
+
+	return ptr; // BAD
+}
+
+MyClass *test3()
+{
+	MyClass mc;
+	MyClass *ptr = &mc;
+	ptr = nullptr;
+	return ptr; // GOOD
+}
+
+MyClass *test4()
+{
+	MyClass mc;
+	MyClass &ref = mc;
+
+	return &ref; // BAD [NOT DETECTED]
+}
+
+MyClass &test5()
+{
+	MyClass mc;
+	return mc; // BAD
+}
+
+int *test6()
+{
+	MyClass mc;
+
+	return &(mc.a); // BAD
+}
+
+MyClass test7()
+{
+	MyClass mc;
+
+	return mc; // GOOD
+}
+
+MyClass *test8()
+{
+	MyClass *mc = new MyClass;
+
+	return mc; // GOOD
+}
+
+MyClass test9()
+{
+	return MyClass(); // GOOD
+}
+
+int test10()
+{
+	MyClass mc;
+
+	return mc.a; // GOOD
+}
+
+MyClass *test11()
+{
+	MyClass *ptr;
+
+	{
+		MyClass mc;
+		ptr = &mc;
+	}
+
+	return ptr; // BAD
+}
+
+MyClass *test12(MyClass *param)
+{
+	return param; // GOOD
+}
+
+MyClass *test13()
+{
+	static MyClass mc;
+	MyClass &ref = mc;
+
+	return &ref; // GOOD
+}
+
+char *testArray1()
+{
+	char arr[256];
+
+	return arr; // BAD
+}
+
+char *testArray2()
+{
+	char arr[256];
+
+	return &(arr[10]); // BAD
+}
+
+char testArray3()
+{
+	char arr[256];
+
+	return arr[10]; // GOOD
+}
+
+char *testArray4()
+{
+	char arr[256];
+	char *ptr;
+
+	ptr = arr + 1;
+	ptr++;
+
+	return ptr; // BAD [NOT DETECTED]
+}
+
+char *testArray5()
+{
+	static char arr[256];
+
+	return arr; // GOOD
+}