Merge pull request #819 from geoffw0/newdelete

CPP: Improve dataflow in newdelete.qll

change-notes/1.20/analysis-cpp.md

@@ -23,6 +23,9 @@
 | Lossy function result cast (`cpp/lossy-function-result-cast`) | Fewer false positive results | The whitelist of rounding functions built into this query has been expanded. |
 | Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. |
 | Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Fix false positives where a resource is released via a virtual method call, function pointer, or lambda. |
+| 'new[]' array freed with 'delete' (`cpp/new-array-delete-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
+| 'new' object freed with 'delete[]' (`cpp/new-delete-array-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
+| Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
 | Use of inherently dangerous function (`cpp/potential-buffer-overflow`) | Cleaned up | This query no longer catches uses of `gets`, and has been renamed 'Potential buffer overflow'. |
 | Use of potentially dangerous function (`cpp/potentially-dangerous-function`) | More correct results | This query now catches uses of `gets`. |
 

cpp/ql/src/Critical/NewDelete.qll

@@ -52,19 +52,26 @@ predicate allocExprOrIndirect(Expr alloc, string kind) {
 }
 
 /**
- * Holds if `v` is a non-local variable which is assigned with
- * memory allocation `alloc` only (it may also be assigned with
- * NULL).  `kind` is a string describing the type of that allocation.
+ * Holds if `v` is assigned value `e`, and `e` is not known to be `0`.
+ */
+private predicate nonNullGlobalAssignment(Variable v, Expr e) {
+  not v instanceof LocalScopeVariable and
+  v.getAnAssignedValue() = e and
+  not e.getValue().toInt() = 0
+}
+
+/**
+ * Holds if `v` is a non-local variable which is assigned only with allocations of
+ * type `kind` (it may also be assigned with NULL).
  */
 private predicate allocReachesVariable(Variable v, Expr alloc, string kind) {
   exists(Expr mid |
-    allocReaches(mid, alloc, kind) and
-    v.getAnAssignedValue() = mid and
-    not v instanceof LocalScopeVariable and
-    count(Expr e |
-      v.getAnAssignedValue() = e and
-      not e.getValue().toInt() = 0
-    ) = 1
+    nonNullGlobalAssignment(v, mid) and
+    allocReaches(mid, alloc, kind)
+  ) and
+  forall(Expr mid |
+    nonNullGlobalAssignment(v, mid) |
+    allocReaches(mid, _, kind)
   )
 }
 

cpp/ql/test/query-tests/Critical/NewFree/NewArrayDeleteMismatch.expected

@@ -2,3 +2,6 @@
 | test.cpp:182:3:182:22 | delete | This memory may have been allocated with '$@', not 'new'. | test.cpp:175:18:175:29 | new[] | new[] |
 | test.cpp:240:2:240:9 | delete | This memory may have been allocated with '$@', not 'new'. | test.cpp:228:7:228:17 | new[] | new[] |
 | test.cpp:295:2:295:11 | delete | This memory may have been allocated with '$@', not 'new'. | test.cpp:290:8:290:28 | new[] | new[] |
+| test.cpp:310:3:310:13 | delete | This memory may have been allocated with '$@', not 'new'. | test.cpp:304:18:304:29 | new[] | new[] |
+| test.cpp:335:3:335:13 | delete | This memory may have been allocated with '$@', not 'new'. | test.cpp:321:10:321:21 | new[] | new[] |
+| test.cpp:335:3:335:13 | delete | This memory may have been allocated with '$@', not 'new'. | test.cpp:329:11:329:24 | new[] | new[] |

cpp/ql/test/query-tests/Critical/NewFree/test.cpp

@@ -295,3 +295,45 @@ static void map_shutdown()
 	delete map; // BAD: new[] -> delete
 	map = 0;
 }
+
+// ---
+
+class Test10
+{
+public:
+	Test10() : data(new char[10])
+	{
+	}
+
+	~Test10()
+	{
+		delete data; // BAD: new[] -> delete
+	}
+
+	char *data;
+};
+
+class Test11
+{
+public:
+	Test11()
+	{
+		data = new char[10];
+	}
+
+	void resize(int size)
+	{
+		if (size > 0)
+		{
+			delete [] data; // GOOD
+			data = new char[size];
+		}
+	}
+
+	~Test11()
+	{
+		delete data; // BAD: new[] -> delete
+	}
+
+	char *data;
+};