From 263e51f72e1b422a27d732a84c3ffb3534f33b31 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 12 Mar 2020 10:12:57 +0000
Subject: [PATCH 01/16] C++: Clean up the test.

---
 .../TaintedAllocationSize.expected                 |  3 ++-
 .../CWE-190/semmle/TaintedAllocationSize/test.cpp  | 14 +++++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
index f77bf73d52d..9b5811ebda5 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
@@ -6,4 +6,5 @@
 | test.cpp:49:17:49:30 | new[] | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
 | test.cpp:52:21:52:27 | call to realloc | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
 | test.cpp:52:35:52:60 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
-| test.cpp:127:17:127:22 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:123:25:123:30 | call to getenv | user input (getenv) |
+| test.cpp:127:17:127:22 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) |
+| test.cpp:127:24:127:41 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
index 5cd5f0c0246..9998b751c53 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
@@ -39,10 +39,10 @@ int main(int argc, char **argv) {
 	int tainted = atoi(argv[1]);
 
 	MyStruct *arr1 = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD
-	MyStruct *arr2 = (MyStruct *)malloc(tainted); // BAD
+	MyStruct *arr2 = (MyStruct *)malloc(tainted); // DUBIOUS (not multiplied by anything)
 	MyStruct *arr3 = (MyStruct *)malloc(tainted * sizeof(MyStruct)); // BAD
 	MyStruct *arr4 = (MyStruct *)malloc(getTainted() * sizeof(MyStruct)); // BAD [NOT DETECTED]
-	MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // BAD [NOT DETECTED]
+	MyStruct *arr5 = (MyStruct *)malloc(sizeof(MyStruct) + tainted); // DUBIOUS (not multiplied by anything)
 
 	int size = tainted * 8;
 	char *chars1 = (char *)malloc(size); // BAD
@@ -52,7 +52,7 @@ int main(int argc, char **argv) {
 	arr1 = (MyStruct *)realloc(arr1, sizeof(MyStruct) * tainted); // BAD
 
 	size = 8;
-	chars3 = new char[size]; // GOOD [FALSE POSITIVE]
+	chars3 = new char[size]; // GOOD
 
 	return 0;
 }
@@ -120,9 +120,9 @@ int bounded(int x, int limit) {
 }
 
 void open_file_bounded () {
-	int size = size = atoi(getenv("USER"));
+	int size = atoi(getenv("USER"));
 	int bounded_size = bounded(size, MAX_SIZE);
 	
-	int* a = (int*)malloc(bounded_size); // GOOD
-	int* b = (int*)malloc(size); // BAD
-}
\ No newline at end of file
+	int* a = (int*)malloc(bounded_size * sizeof(int)); // GOOD
+	int* b = (int*)malloc(size * sizeof(int)); // BAD
+}

From 26ed560bd7020ff0879bdee0f059b5e74ee12d7c Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 12 Mar 2020 10:49:20 +0000
Subject: [PATCH 02/16] C++: Add new test cases.

---
 .../TaintedAllocationSize.expected            |  6 ++
 .../semmle/TaintedAllocationSize/test.cpp     | 64 +++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
index 9b5811ebda5..8568cca660f 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
@@ -8,3 +8,9 @@
 | test.cpp:52:35:52:60 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:39:21:39:24 | argv | user input (argv) |
 | test.cpp:127:17:127:22 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) |
 | test.cpp:127:24:127:41 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:123:18:123:23 | call to getenv | user input (getenv) |
+| test.cpp:134:3:134:8 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) |
+| test.cpp:134:10:134:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) |
+| test.cpp:142:4:142:9 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
+| test.cpp:142:11:142:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
+| test.cpp:169:4:169:9 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:165:19:165:24 | call to getenv | user input (getenv) |
+| test.cpp:169:11:169:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:165:19:165:24 | call to getenv | user input (getenv) |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
index 9998b751c53..317bbfc7c19 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
@@ -126,3 +126,67 @@ void open_file_bounded () {
 	int* a = (int*)malloc(bounded_size * sizeof(int)); // GOOD
 	int* b = (int*)malloc(size * sizeof(int)); // BAD
 }
+
+void more_bounded_tests() {
+	{
+		int size = atoi(getenv("USER"));
+
+		malloc(size * sizeof(int)); // BAD
+	}
+
+	{
+		int size = atoi(getenv("USER"));
+
+		if (size > 0)
+		{
+			malloc(size * sizeof(int)); // BAD
+		}
+	}
+
+	{
+		int size = atoi(getenv("USER"));
+
+		if (size < 100)
+		{
+			malloc(size * sizeof(int)); // BAD [NOT DETECTED]
+		}
+	}
+
+	{
+		int size = atoi(getenv("USER"));
+
+		if ((size > 0) && (size < 100))
+		{
+			malloc(size * sizeof(int)); // GOOD
+		}
+	}
+
+	{
+		int size = atoi(getenv("USER"));
+
+		if ((100 > size) && (0 < size))
+		{
+			malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE]
+		}
+	}
+
+	{
+		int size = atoi(getenv("USER"));
+
+		malloc(size * sizeof(int)); // BAD [NOT DETECTED]
+
+		if ((size > 0) && (size < 100))
+		{
+			// ...
+		}
+	}
+
+	{
+		int size = atoi(getenv("USER"));
+
+		if (size > 100)
+		{
+			malloc(size * sizeof(int)); // BAD [NOT DETECTED]
+		}
+	}
+}

From f4a1b410947460611ae7bbd0ab06c52cbfcfa280 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 12 Mar 2020 12:36:09 +0000
Subject: [PATCH 03/16] C++: Correct hasUpperBoundsCheck.

---
 .../cpp/ir/dataflow/DefaultTaintTracking.qll     | 16 +++++++++++++---
 .../code/cpp/security/TaintTrackingImpl.qll      | 16 +++++++++++++---
 .../TaintedAllocationSize.expected               |  2 --
 .../semmle/TaintedAllocationSize/test.cpp        |  2 +-
 4 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
index e78d4d5d288..0a38b27ee4e 100644
--- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
@@ -130,7 +130,17 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
 }
 
 /**
- * A variable that has any kind of upper-bound check anywhere in the program
+ * A variable that has any kind of upper-bound check anywhere in the program.  This is
+ * biased towards being inclusive because there are a lot of valid ways of doing an
+ * upper bounds checks if we don't consider where it occurs, for example:
+ * ```
+ *   if (x < 10) { sink(x); }
+ * 
+ *   if (10 > y) { sink(y); }
+ *
+ *   if (z > 10) { z = 10; }
+ *   sink(z);
+ * ```
  */
 // TODO: This coarse overapproximation, ported from the old taint tracking
 // library, could be replaced with an actual semantic check that a particular
@@ -139,10 +149,10 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
 // previously suppressed by this predicate by coincidence.
 private predicate hasUpperBoundsCheck(Variable var) {
   exists(RelationalOperation oper, VariableAccess access |
-    oper.getLeftOperand() = access and
+    oper.getAnOperand() = access and
     access.getTarget() = var and
     // Comparing to 0 is not an upper bound check
-    not oper.getRightOperand().getValue() = "0"
+    not oper.getAnOperand().getValue() = "0"
   )
 }
 
diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
index 3a37b43b319..c0d9ef6a912 100644
--- a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
@@ -328,14 +328,24 @@ GlobalOrNamespaceVariable globalVarFromId(string id) {
 }
 
 /**
- * A variable that has any kind of upper-bound check anywhere in the program
+ * A variable that has any kind of upper-bound check anywhere in the program.  This is
+ * biased towards being inclusive because there are a lot of valid ways of doing an
+ * upper bounds checks if we don't consider where it occurs, for example:
+ * ```
+ *   if (x < 10) { sink(x); }
+ * 
+ *   if (10 > y) { sink(y); }
+ *
+ *   if (z > 10) { z = 10; }
+ *   sink(z);
+ * ```
  */
 private predicate hasUpperBoundsCheck(Variable var) {
   exists(RelationalOperation oper, VariableAccess access |
-    oper.getLeftOperand() = access and
+    oper.getAnOperand() = access and
     access.getTarget() = var and
     // Comparing to 0 is not an upper bound check
-    not oper.getRightOperand().getValue() = "0"
+    not oper.getAnOperand().getValue() = "0"
   )
 }
 
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
index 8568cca660f..c48e2071525 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
@@ -12,5 +12,3 @@
 | test.cpp:134:10:134:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) |
 | test.cpp:142:4:142:9 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
 | test.cpp:142:11:142:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
-| test.cpp:169:4:169:9 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:165:19:165:24 | call to getenv | user input (getenv) |
-| test.cpp:169:11:169:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:165:19:165:24 | call to getenv | user input (getenv) |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
index 317bbfc7c19..4a1bbd8a9dc 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp
@@ -166,7 +166,7 @@ void more_bounded_tests() {
 
 		if ((100 > size) && (0 < size))
 		{
-			malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE]
+			malloc(size * sizeof(int)); // GOOD
 		}
 	}
 

From cecbdae3e1d8e6066033c9ccca95b7ffd5fa61a5 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 13 Mar 2020 17:58:31 +0000
Subject: [PATCH 04/16] C++: Change note.

---
 change-notes/1.24/analysis-cpp.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md
index b4024b73307..c07d2004b74 100644
--- a/change-notes/1.24/analysis-cpp.md
+++ b/change-notes/1.24/analysis-cpp.md
@@ -22,6 +22,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
+| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
 | Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) |  | This query is no longer run on LGTM. |
 

From 97cdcbee6388947c3a66da8a0cd5e6ec37d520b9 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 13:52:16 +0100
Subject: [PATCH 05/16] C++: Test for NewFreeMismatch.ql with operator new /
 delete.

---
 .../Critical/NewFree/NewFreeMismatch.expected |  4 ++++
 .../query-tests/Critical/NewFree/test2.cpp    | 24 +++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
index 7350749ce96..d588fc26e7b 100644
--- a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
+++ b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
@@ -1,5 +1,9 @@
 | test2.cpp:19:3:19:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:18:12:18:18 | new | new |
 | test2.cpp:26:3:26:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:25:7:25:13 | new | new |
+| test2.cpp:50:2:50:18 | call to operator delete | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new |
+| test2.cpp:51:2:51:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new |
+| test2.cpp:53:2:53:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:46:20:46:33 | call to operator new | malloc |
+| test2.cpp:57:2:57:18 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc |
 | test.cpp:36:2:36:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:27:18:27:23 | call to malloc | malloc |
 | test.cpp:41:2:41:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test.cpp:26:7:26:17 | new | new |
 | test.cpp:68:3:68:11 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:64:28:64:33 | call to malloc | malloc |
diff --git a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
index 9101758d85b..15ab818d2c6 100644
--- a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
+++ b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
@@ -34,3 +34,27 @@ public:
 };
 
 MyTest2Class<int> mt2c_i;
+
+// ---
+
+void* operator new(size_t);
+void operator delete(void*);
+
+void test_operator_new()
+{
+	void *ptr_new = new int;
+	void *ptr_opnew = ::operator new(sizeof(int));
+	void *ptr_malloc = malloc(sizeof(int));
+
+	delete ptr_new; // GOOD
+	::operator delete(ptr_new); // GOOD [FALSE POSITIVE]
+	free(ptr_new); // BAD
+
+	delete ptr_opnew; // GOOD [FALSE POSITIVE]
+	::operator delete(ptr_opnew); // GOOD
+	free(ptr_opnew); // BAD [NOT DETECTED]
+
+	delete ptr_malloc; // BAD
+	::operator delete(ptr_malloc); // BAD [NOT DETECTED]
+	free(ptr_malloc); // GOOD
+}

From 3e9f9645aecd8ad778ff9bcc97f098da0e7b985c Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 11:30:14 +0100
Subject: [PATCH 06/16] C++: Exclude calls to operator new / delete from
 NewFreeMismatch.ql.

---
 cpp/ql/src/Critical/NewDelete.qll                             | 4 ++++
 .../query-tests/Critical/NewFree/NewFreeMismatch.expected     | 2 --
 cpp/ql/test/query-tests/Critical/NewFree/test2.cpp            | 4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll
index 9dd55525b59..7650af2f053 100644
--- a/cpp/ql/src/Critical/NewDelete.qll
+++ b/cpp/ql/src/Critical/NewDelete.qll
@@ -5,6 +5,8 @@
 import cpp
 import semmle.code.cpp.controlflow.SSA
 import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.models.implementations.Allocation
+import semmle.code.cpp.models.implementations.Deallocation
 
 /**
  * Holds if `alloc` is a use of `malloc` or `new`.  `kind` is
@@ -15,6 +17,7 @@ predicate allocExpr(Expr alloc, string kind) {
   not alloc.isFromUninstantiatedTemplate(_) and
   (
     alloc instanceof FunctionCall and
+    not alloc.(FunctionCall).getTarget() instanceof OperatorNewAllocationFunction and
     kind = "malloc"
     or
     alloc instanceof NewExpr and
@@ -111,6 +114,7 @@ predicate allocReaches(Expr e, Expr alloc, string kind) {
  */
 predicate freeExpr(Expr free, Expr freed, string kind) {
   freeCall(free, freed) and
+  not free.(FunctionCall).getTarget() instanceof OperatorDeleteDeallocationFunction and
   kind = "free"
   or
   free.(DeleteExpr).getExpr() = freed and
diff --git a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
index d588fc26e7b..8bcb9efa587 100644
--- a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
+++ b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
@@ -1,8 +1,6 @@
 | test2.cpp:19:3:19:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:18:12:18:18 | new | new |
 | test2.cpp:26:3:26:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:25:7:25:13 | new | new |
-| test2.cpp:50:2:50:18 | call to operator delete | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new |
 | test2.cpp:51:2:51:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new |
-| test2.cpp:53:2:53:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:46:20:46:33 | call to operator new | malloc |
 | test2.cpp:57:2:57:18 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc |
 | test.cpp:36:2:36:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:27:18:27:23 | call to malloc | malloc |
 | test.cpp:41:2:41:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test.cpp:26:7:26:17 | new | new |
diff --git a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
index 15ab818d2c6..d5abd62ab71 100644
--- a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
+++ b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
@@ -47,10 +47,10 @@ void test_operator_new()
 	void *ptr_malloc = malloc(sizeof(int));
 
 	delete ptr_new; // GOOD
-	::operator delete(ptr_new); // GOOD [FALSE POSITIVE]
+	::operator delete(ptr_new); // GOOD
 	free(ptr_new); // BAD
 
-	delete ptr_opnew; // GOOD [FALSE POSITIVE]
+	delete ptr_opnew; // GOOD
 	::operator delete(ptr_opnew); // GOOD
 	free(ptr_opnew); // BAD [NOT DETECTED]
 

From 8059d69bbd754b7d26e93cfc109254e680e03b98 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 14:24:16 +0100
Subject: [PATCH 07/16] C++: Model calls to operator new / delete for
 NewFreeMismatch.ql.

---
 cpp/ql/src/Critical/NewDelete.qll             | 33 +++++++++++++++----
 .../Critical/NewFree/NewFreeMismatch.expected |  2 ++
 .../query-tests/Critical/NewFree/test2.cpp    |  4 +--
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll
index 7650af2f053..93d4ca0973c 100644
--- a/cpp/ql/src/Critical/NewDelete.qll
+++ b/cpp/ql/src/Critical/NewDelete.qll
@@ -16,9 +16,19 @@ predicate allocExpr(Expr alloc, string kind) {
   isAllocationExpr(alloc) and
   not alloc.isFromUninstantiatedTemplate(_) and
   (
-    alloc instanceof FunctionCall and
-    not alloc.(FunctionCall).getTarget() instanceof OperatorNewAllocationFunction and
-    kind = "malloc"
+    exists(Function target |
+      alloc.(FunctionCall).getTarget() = target and
+      (
+        target.getName() = "operator new" and
+        kind = "new"
+        or
+        target.getName() = "operator new[]" and
+        kind = "new[]"
+        or
+        not target instanceof OperatorNewAllocationFunction and
+        kind = "malloc"
+      )
+    )
     or
     alloc instanceof NewExpr and
     kind = "new" and
@@ -113,9 +123,20 @@ predicate allocReaches(Expr e, Expr alloc, string kind) {
  * describing the type of that free or delete.
  */
 predicate freeExpr(Expr free, Expr freed, string kind) {
-  freeCall(free, freed) and
-  not free.(FunctionCall).getTarget() instanceof OperatorDeleteDeallocationFunction and
-  kind = "free"
+  exists(Function target |
+    freeCall(free, freed) and
+    free.(FunctionCall).getTarget() = target and
+    (
+      target.getName() = "operator delete" and
+      kind = "delete"
+      or
+      target.getName() = "operator delete[]" and
+      kind = "delete[]"
+      or
+      not target instanceof OperatorDeleteDeallocationFunction and
+      kind = "free"
+    )
+  )
   or
   free.(DeleteExpr).getExpr() = freed and
   kind = "delete"
diff --git a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
index 8bcb9efa587..45f88d426c9 100644
--- a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
+++ b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected
@@ -1,7 +1,9 @@
 | test2.cpp:19:3:19:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:18:12:18:18 | new | new |
 | test2.cpp:26:3:26:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:25:7:25:13 | new | new |
 | test2.cpp:51:2:51:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new |
+| test2.cpp:55:2:55:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:46:20:46:33 | call to operator new | new |
 | test2.cpp:57:2:57:18 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc |
+| test2.cpp:58:2:58:18 | call to operator delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc |
 | test.cpp:36:2:36:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:27:18:27:23 | call to malloc | malloc |
 | test.cpp:41:2:41:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test.cpp:26:7:26:17 | new | new |
 | test.cpp:68:3:68:11 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:64:28:64:33 | call to malloc | malloc |
diff --git a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
index d5abd62ab71..43a286f6f97 100644
--- a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
+++ b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
@@ -52,9 +52,9 @@ void test_operator_new()
 
 	delete ptr_opnew; // GOOD
 	::operator delete(ptr_opnew); // GOOD
-	free(ptr_opnew); // BAD [NOT DETECTED]
+	free(ptr_opnew); // BAD
 
 	delete ptr_malloc; // BAD
-	::operator delete(ptr_malloc); // BAD [NOT DETECTED]
+	::operator delete(ptr_malloc); // BAD
 	free(ptr_malloc); // GOOD
 }

From e223557201d5326af4ee91c59a28815fc5694695 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 14:32:15 +0100
Subject: [PATCH 08/16] C++: Wean NewDelete.qll off the legacy wrapper
 Alloc.qll.

---
 cpp/ql/src/Critical/NewDelete.qll | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll
index 93d4ca0973c..1670dfc1357 100644
--- a/cpp/ql/src/Critical/NewDelete.qll
+++ b/cpp/ql/src/Critical/NewDelete.qll
@@ -13,8 +13,11 @@ import semmle.code.cpp.models.implementations.Deallocation
  * a string describing the type of the allocation.
  */
 predicate allocExpr(Expr alloc, string kind) {
-  isAllocationExpr(alloc) and
-  not alloc.isFromUninstantiatedTemplate(_) and
+  (
+    alloc.(FunctionCall) instanceof AllocationExpr
+    or
+    alloc = any(NewOrNewArrayExpr new | not exists(new.getPlacementPointer()))
+  ) and
   (
     exists(Function target |
       alloc.(FunctionCall).getTarget() = target and
@@ -41,7 +44,8 @@ predicate allocExpr(Expr alloc, string kind) {
     // exclude placement new and custom overloads as they
     // may not conform to assumptions
     not alloc.(NewArrayExpr).getAllocatorCall().getTarget().getNumberOfParameters() > 1
-  )
+  ) and
+  not alloc.isFromUninstantiatedTemplate(_)
 }
 
 /**
@@ -124,7 +128,7 @@ predicate allocReaches(Expr e, Expr alloc, string kind) {
  */
 predicate freeExpr(Expr free, Expr freed, string kind) {
   exists(Function target |
-    freeCall(free, freed) and
+    freed = free.(DeallocationExpr).getFreedExpr() and
     free.(FunctionCall).getTarget() = target and
     (
       target.getName() = "operator delete" and

From cbe133d0e6da1b8b1a6232818c84464307eda974 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 14:32:49 +0100
Subject: [PATCH 09/16] C++: Deprecate freeCall in the legacy wrapper
 Alloc.qll.

---
 cpp/ql/src/Critical/MemoryFreed.qll          | 2 +-
 cpp/ql/src/semmle/code/cpp/commons/Alloc.qll | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cpp/ql/src/Critical/MemoryFreed.qll b/cpp/ql/src/Critical/MemoryFreed.qll
index d548aca26e7..880199e54c9 100644
--- a/cpp/ql/src/Critical/MemoryFreed.qll
+++ b/cpp/ql/src/Critical/MemoryFreed.qll
@@ -4,7 +4,7 @@ private predicate freed(Expr e) {
   e = any(DeallocationExpr de).getFreedExpr()
   or
   exists(ExprCall c |
-    // cautiously assume that any ExprCall could be a freeCall.
+    // cautiously assume that any `ExprCall` could be a deallocation expression.
     c.getAnArgument() = e
   )
 }
diff --git a/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll b/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll
index a508c929d9a..a9597fc72b5 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll
@@ -23,6 +23,8 @@ predicate freeFunction(Function f, int argNum) { argNum = f.(DeallocationFunctio
 
 /**
  * A call to a library routine that frees memory.
+ *
+ * DEPRECATED: Use `DeallocationExpr` instead (this also includes `delete` expressions).
  */
 predicate freeCall(FunctionCall fc, Expr arg) { arg = fc.(DeallocationExpr).getFreedExpr() }
 

From 050e2395072a38c9ab8834d6c360d6aa3331a780 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 14:37:47 +0100
Subject: [PATCH 10/16] C++: Change note.

---
 change-notes/1.24/analysis-cpp.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md
index 2b53e6d56aa..73d58a57404 100644
--- a/change-notes/1.24/analysis-cpp.md
+++ b/change-notes/1.24/analysis-cpp.md
@@ -53,3 +53,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
   * The library now models data flow through formatting functions such as `sprintf`.
 * The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) uses a new intermediate representation. This provides a more precise analysis of pointers to stack variables and flow through parameters, improving the results of many security queries.
 * The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering`) uses a new intermediate representation to provide a more precise analysis of heap allocated memory and pointers to stack variables.
+* `freeCall` in `semmle.code.cpp.commons.Alloc` has been deprecated. The`Allocation` and `Deallocation` models in `semmle.code.cpp.models.interfaces` should be used instead.

From 492c5f367f7f4c4fe142c9e6d79040df0738ffbe Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 14:44:38 +0100
Subject: [PATCH 11/16] C++: Simplify NewDelete.qll.

---
 cpp/ql/src/Critical/NewDelete.qll | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll
index 1670dfc1357..b9c84c04a3a 100644
--- a/cpp/ql/src/Critical/NewDelete.qll
+++ b/cpp/ql/src/Critical/NewDelete.qll
@@ -13,14 +13,9 @@ import semmle.code.cpp.models.implementations.Deallocation
  * a string describing the type of the allocation.
  */
 predicate allocExpr(Expr alloc, string kind) {
-  (
-    alloc.(FunctionCall) instanceof AllocationExpr
-    or
-    alloc = any(NewOrNewArrayExpr new | not exists(new.getPlacementPointer()))
-  ) and
   (
     exists(Function target |
-      alloc.(FunctionCall).getTarget() = target and
+      alloc.(AllocationExpr).(FunctionCall).getTarget() = target and
       (
         target.getName() = "operator new" and
         kind = "new"

From a71ae2b4687d60818f041a3aa1e5bd240b2d2f30 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 6 Apr 2020 14:46:00 +0100
Subject: [PATCH 12/16] C++: Consistent treatment of placement new.

---
 cpp/ql/src/Critical/NewDelete.qll | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll
index b9c84c04a3a..30b9f9ad94a 100644
--- a/cpp/ql/src/Critical/NewDelete.qll
+++ b/cpp/ql/src/Critical/NewDelete.qll
@@ -18,10 +18,16 @@ predicate allocExpr(Expr alloc, string kind) {
       alloc.(AllocationExpr).(FunctionCall).getTarget() = target and
       (
         target.getName() = "operator new" and
-        kind = "new"
+        kind = "new" and
+        // exclude placement new and custom overloads as they
+        // may not conform to assumptions
+        not target.getNumberOfParameters() > 1
         or
         target.getName() = "operator new[]" and
-        kind = "new[]"
+        kind = "new[]" and
+        // exclude placement new and custom overloads as they
+        // may not conform to assumptions
+        not target.getNumberOfParameters() > 1
         or
         not target instanceof OperatorNewAllocationFunction and
         kind = "malloc"

From ff39f714e8adc9bfbe5f07f5dd96d74cacf4b1f3 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 7 Apr 2020 17:07:31 +0100
Subject: [PATCH 13/16] C++: Autoformat.

---
 cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll | 2 +-
 cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
index 0a38b27ee4e..a48efbe9e64 100644
--- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
@@ -135,7 +135,7 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
  * upper bounds checks if we don't consider where it occurs, for example:
  * ```
  *   if (x < 10) { sink(x); }
- * 
+ *
  *   if (10 > y) { sink(y); }
  *
  *   if (z > 10) { z = 10; }
diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
index c0d9ef6a912..a24820b277f 100644
--- a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
@@ -333,7 +333,7 @@ GlobalOrNamespaceVariable globalVarFromId(string id) {
  * upper bounds checks if we don't consider where it occurs, for example:
  * ```
  *   if (x < 10) { sink(x); }
- * 
+ *
  *   if (10 > y) { sink(y); }
  *
  *   if (z > 10) { z = 10; }

From 2686d9888cdfdc12eb14b7685cb806b5d39897ef Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 7 Apr 2020 18:10:56 +0100
Subject: [PATCH 14/16] C++: Add QLDoc.

---
 .../semmle/code/cpp/models/implementations/Allocation.qll | 8 +++++++-
 .../code/cpp/models/implementations/Deallocation.qll      | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll
index c6766983889..d2577dc3ca4 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll
@@ -1,4 +1,10 @@
-import semmle.code.cpp.models.interfaces.Allocation
+/**
+ * Provides implementation classes modelling various methods of allocation
+ * (`malloc`, `new` etc). See `semmle.code.cpp.models.interfaces.Allocation`
+ * for usage information.
+ */
+
+ import semmle.code.cpp.models.interfaces.Allocation
 
 /**
  * An allocation function (such as `malloc`) that has an argument for the size
diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
index d2e4951e436..6637c6e1b26 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
@@ -1,4 +1,10 @@
-import semmle.code.cpp.models.interfaces.Allocation
+/**
+ * Provides implementation classes  modelling various methods of deallocation
+ * (`free`, `delete` etc). See `semmle.code.cpp.models.interfaces.Deallocation`
+ * for usage information.
+ */
+
+ import semmle.code.cpp.models.interfaces.Allocation
 
 /**
  * A deallocation function such as `free`.

From 50194f372b8a55bce887288d7916837873ef89b8 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 7 Apr 2020 20:54:54 +0100
Subject: [PATCH 15/16] C++: Autoformat.

---
 .../src/semmle/code/cpp/models/implementations/Allocation.qll   | 2 +-
 .../src/semmle/code/cpp/models/implementations/Deallocation.qll | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll
index d2577dc3ca4..f6f7ab279a6 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll
@@ -4,7 +4,7 @@
  * for usage information.
  */
 
- import semmle.code.cpp.models.interfaces.Allocation
+import semmle.code.cpp.models.interfaces.Allocation
 
 /**
  * An allocation function (such as `malloc`) that has an argument for the size
diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
index 6637c6e1b26..60076ed28d7 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
@@ -4,7 +4,7 @@
  * for usage information.
  */
 
- import semmle.code.cpp.models.interfaces.Allocation
+import semmle.code.cpp.models.interfaces.Allocation
 
 /**
  * A deallocation function such as `free`.

From 7fedac3266f8bbcfda9d5892c01eb5c158391746 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 7 Apr 2020 20:56:07 +0100
Subject: [PATCH 16/16] C++: Fix apparently noncritical typo.

---
 .../src/semmle/code/cpp/models/implementations/Deallocation.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
index 60076ed28d7..980645df031 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll
@@ -4,7 +4,7 @@
  * for usage information.
  */
 
-import semmle.code.cpp.models.interfaces.Allocation
+import semmle.code.cpp.models.interfaces.Deallocation
 
 /**
  * A deallocation function such as `free`.