C++: Use fully converted expressions in 'cpp/use-after-free' and 'cpp/double-free'.

Author: MathiasVP

cpp/ql/src/Critical/FlowAfterFree.qll

@@ -98,8 +98,11 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
  * is being freed by a deallocation expression `dealloc`.
  */
 predicate isFree(DataFlow::Node n, Expr e, DeallocationExpr dealloc) {
-  e = dealloc.getFreedExpr() and
-  e = n.asExpr() and
+  exists(Expr conv |
+    e = conv.getUnconverted() and
+    conv = dealloc.getFreedExpr().getFullyConverted() and
+    conv = n.asConvertedExpr()
+  ) and
   // Ignore realloc functions
   not exists(dealloc.(FunctionCall).getTarget().(AllocationFunction).getReallocPtrArg())
 }

cpp/ql/test/query-tests/Critical/MemoryFreed/DoubleFree.expected

@@ -6,13 +6,10 @@ edges
 | test_free.cpp:44:27:44:27 | a | test_free.cpp:46:10:46:10 | a |
 | test_free.cpp:50:27:50:27 | a | test_free.cpp:51:10:51:10 | a |
 | test_free.cpp:69:10:69:10 | a | test_free.cpp:72:14:72:14 | a |
-| test_free.cpp:83:12:83:12 | a | test_free.cpp:85:12:85:12 | a |
 | test_free.cpp:101:10:101:10 | a | test_free.cpp:103:10:103:10 | a |
 | test_free.cpp:128:10:128:11 | * ... | test_free.cpp:129:10:129:11 | * ... |
 | test_free.cpp:152:27:152:27 | a | test_free.cpp:154:10:154:10 | a |
 | test_free.cpp:207:10:207:10 | a | test_free.cpp:209:10:209:10 | a |
-| test_free.cpp:252:7:252:7 | p | test_free.cpp:255:10:255:10 | p |
-| test_free.cpp:260:9:260:9 | p | test_free.cpp:263:12:263:12 | p |
 nodes
 | test_free.cpp:11:10:11:10 | a | semmle.label | a |
 | test_free.cpp:14:10:14:10 | a | semmle.label | a |
@@ -28,8 +25,6 @@ nodes
 | test_free.cpp:51:10:51:10 | a | semmle.label | a |
 | test_free.cpp:69:10:69:10 | a | semmle.label | a |
 | test_free.cpp:72:14:72:14 | a | semmle.label | a |
-| test_free.cpp:83:12:83:12 | a | semmle.label | a |
-| test_free.cpp:85:12:85:12 | a | semmle.label | a |
 | test_free.cpp:101:10:101:10 | a | semmle.label | a |
 | test_free.cpp:103:10:103:10 | a | semmle.label | a |
 | test_free.cpp:128:10:128:11 | * ... | semmle.label | * ... |
@@ -38,10 +33,6 @@ nodes
 | test_free.cpp:154:10:154:10 | a | semmle.label | a |
 | test_free.cpp:207:10:207:10 | a | semmle.label | a |
 | test_free.cpp:209:10:209:10 | a | semmle.label | a |
-| test_free.cpp:252:7:252:7 | p | semmle.label | p |
-| test_free.cpp:255:10:255:10 | p | semmle.label | p |
-| test_free.cpp:260:9:260:9 | p | semmle.label | p |
-| test_free.cpp:263:12:263:12 | p | semmle.label | p |
 subpaths
 #select
 | test_free.cpp:14:10:14:10 | a | test_free.cpp:11:10:11:10 | a | test_free.cpp:14:10:14:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:11:5:11:8 | call to free | call to free |
@@ -51,10 +42,7 @@ subpaths
 | test_free.cpp:46:10:46:10 | a | test_free.cpp:44:27:44:27 | a | test_free.cpp:46:10:46:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:44:22:44:25 | call to free | call to free |
 | test_free.cpp:51:10:51:10 | a | test_free.cpp:50:27:50:27 | a | test_free.cpp:51:10:51:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:50:22:50:25 | call to free | call to free |
 | test_free.cpp:72:14:72:14 | a | test_free.cpp:69:10:69:10 | a | test_free.cpp:72:14:72:14 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:69:5:69:8 | call to free | call to free |
-| test_free.cpp:85:12:85:12 | a | test_free.cpp:83:12:83:12 | a | test_free.cpp:85:12:85:12 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:83:5:83:13 | delete | delete |
 | test_free.cpp:103:10:103:10 | a | test_free.cpp:101:10:101:10 | a | test_free.cpp:103:10:103:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:101:5:101:8 | call to free | call to free |
 | test_free.cpp:129:10:129:11 | * ... | test_free.cpp:128:10:128:11 | * ... | test_free.cpp:129:10:129:11 | * ... | Memory pointed to by '* ...' may already have been freed by $@. | test_free.cpp:128:5:128:8 | call to free | call to free |
 | test_free.cpp:154:10:154:10 | a | test_free.cpp:152:27:152:27 | a | test_free.cpp:154:10:154:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:152:22:152:25 | call to free | call to free |
 | test_free.cpp:209:10:209:10 | a | test_free.cpp:207:10:207:10 | a | test_free.cpp:209:10:209:10 | a | Memory pointed to by 'a' may already have been freed by $@. | test_free.cpp:207:5:207:8 | call to free | call to free |
-| test_free.cpp:255:10:255:10 | p | test_free.cpp:252:7:252:7 | p | test_free.cpp:255:10:255:10 | p | Memory pointed to by 'p' may already have been freed by $@. | test_free.cpp:252:2:252:5 | call to free | call to free |
-| test_free.cpp:263:12:263:12 | p | test_free.cpp:260:9:260:9 | p | test_free.cpp:263:12:263:12 | p | Memory pointed to by 'p' may already have been freed by $@. | test_free.cpp:260:2:260:9 | delete | delete |

cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected

@@ -4,16 +4,13 @@ edges
 | test_free.cpp:42:27:42:27 | a | test_free.cpp:45:5:45:5 | a |
 | test_free.cpp:44:27:44:27 | a | test_free.cpp:45:5:45:5 | a |
 | test_free.cpp:69:10:69:10 | a | test_free.cpp:71:9:71:9 | a |
-| test_free.cpp:83:12:83:12 | a | test_free.cpp:84:5:84:5 | a |
 | test_free.cpp:90:10:90:10 | a | test_free.cpp:91:5:91:5 | a |
 | test_free.cpp:95:10:95:10 | a | test_free.cpp:96:9:96:9 | a |
 | test_free.cpp:101:10:101:10 | a | test_free.cpp:102:23:102:23 | a |
 | test_free.cpp:152:27:152:27 | a | test_free.cpp:153:5:153:5 | a |
 | test_free.cpp:233:14:233:15 | * ... | test_free.cpp:236:9:236:10 | * ... |
 | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... |
 | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... |
-| test_free.cpp:252:7:252:7 | p | test_free.cpp:254:6:254:6 | p |
-| test_free.cpp:260:9:260:9 | p | test_free.cpp:262:6:262:6 | p |
 nodes
 | test_free.cpp:11:10:11:10 | a | semmle.label | a |
 | test_free.cpp:12:5:12:5 | a | semmle.label | a |
@@ -24,8 +21,6 @@ nodes
 | test_free.cpp:45:5:45:5 | a | semmle.label | a |
 | test_free.cpp:69:10:69:10 | a | semmle.label | a |
 | test_free.cpp:71:9:71:9 | a | semmle.label | a |
-| test_free.cpp:83:12:83:12 | a | semmle.label | a |
-| test_free.cpp:84:5:84:5 | a | semmle.label | a |
 | test_free.cpp:90:10:90:10 | a | semmle.label | a |
 | test_free.cpp:91:5:91:5 | a | semmle.label | a |
 | test_free.cpp:95:10:95:10 | a | semmle.label | a |
@@ -40,24 +35,17 @@ nodes
 | test_free.cpp:241:9:241:10 | * ... | semmle.label | * ... |
 | test_free.cpp:245:10:245:11 | * ... | semmle.label | * ... |
 | test_free.cpp:246:9:246:10 | * ... | semmle.label | * ... |
-| test_free.cpp:252:7:252:7 | p | semmle.label | p |
-| test_free.cpp:254:6:254:6 | p | semmle.label | p |
-| test_free.cpp:260:9:260:9 | p | semmle.label | p |
-| test_free.cpp:262:6:262:6 | p | semmle.label | p |
 subpaths
 #select
 | test_free.cpp:12:5:12:5 | a | test_free.cpp:11:10:11:10 | a | test_free.cpp:12:5:12:5 | a | Memory may have been previously freed by $@. | test_free.cpp:11:5:11:8 | call to free | call to free |
 | test_free.cpp:13:5:13:6 | * ... | test_free.cpp:11:10:11:10 | a | test_free.cpp:13:5:13:6 | * ... | Memory may have been previously freed by $@. | test_free.cpp:11:5:11:8 | call to free | call to free |
 | test_free.cpp:45:5:45:5 | a | test_free.cpp:42:27:42:27 | a | test_free.cpp:45:5:45:5 | a | Memory may have been previously freed by $@. | test_free.cpp:42:22:42:25 | call to free | call to free |
 | test_free.cpp:45:5:45:5 | a | test_free.cpp:44:27:44:27 | a | test_free.cpp:45:5:45:5 | a | Memory may have been previously freed by $@. | test_free.cpp:44:22:44:25 | call to free | call to free |
 | test_free.cpp:71:9:71:9 | a | test_free.cpp:69:10:69:10 | a | test_free.cpp:71:9:71:9 | a | Memory may have been previously freed by $@. | test_free.cpp:69:5:69:8 | call to free | call to free |
-| test_free.cpp:84:5:84:5 | a | test_free.cpp:83:12:83:12 | a | test_free.cpp:84:5:84:5 | a | Memory may have been previously freed by $@. | test_free.cpp:83:5:83:13 | delete | delete |
 | test_free.cpp:91:5:91:5 | a | test_free.cpp:90:10:90:10 | a | test_free.cpp:91:5:91:5 | a | Memory may have been previously freed by $@. | test_free.cpp:90:5:90:8 | call to free | call to free |
 | test_free.cpp:96:9:96:9 | a | test_free.cpp:95:10:95:10 | a | test_free.cpp:96:9:96:9 | a | Memory may have been previously freed by $@. | test_free.cpp:95:5:95:8 | call to free | call to free |
 | test_free.cpp:102:23:102:23 | a | test_free.cpp:101:10:101:10 | a | test_free.cpp:102:23:102:23 | a | Memory may have been previously freed by $@. | test_free.cpp:101:5:101:8 | call to free | call to free |
 | test_free.cpp:153:5:153:5 | a | test_free.cpp:152:27:152:27 | a | test_free.cpp:153:5:153:5 | a | Memory may have been previously freed by $@. | test_free.cpp:152:22:152:25 | call to free | call to free |
 | test_free.cpp:236:9:236:10 | * ... | test_free.cpp:233:14:233:15 | * ... | test_free.cpp:236:9:236:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:233:9:233:12 | call to free | call to free |
 | test_free.cpp:241:9:241:10 | * ... | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free |
 | test_free.cpp:246:9:246:10 | * ... | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:245:5:245:8 | call to free | call to free |
-| test_free.cpp:254:6:254:6 | p | test_free.cpp:252:7:252:7 | p | test_free.cpp:254:6:254:6 | p | Memory may have been previously freed by $@. | test_free.cpp:252:2:252:5 | call to free | call to free |
-| test_free.cpp:262:6:262:6 | p | test_free.cpp:260:9:260:9 | p | test_free.cpp:262:6:262:6 | p | Memory may have been previously freed by $@. | test_free.cpp:260:2:260:9 | delete | delete |

cpp/ql/test/query-tests/Critical/MemoryFreed/test_free.cpp

@@ -81,8 +81,8 @@ class A {
 void test_new1() {
     A *a = new A();
     delete(a);
-    a->f(); // BAD
-    delete(a); // BAD
+    a->f(); // BAD [NOT DETECTED]
+    delete(a); // BAD [NOT DETECTED]
 }
 
 void test_dereference1(A *a) {
@@ -251,14 +251,14 @@ void test_deref(char **a) {
 void test_ref(char *&p) {
 	free(p);
 	p = (char *)malloc(sizeof(char)*10);
-	use(p);  // GOOD [FALSE POSITIVE]
-    free(p); // GOOD [FALSE POSITIVE]
+	use(p);  // GOOD
+    free(p); // GOOD
 }
 
 
 void test_ref_delete(int *&p) {
 	delete p;
 	p = new int;
-	use(p);  // GOOD [FALSE POSITIVE]
-    delete p;  // GOOD [FALSE POSITIVE]
+	use(p);  // GOOD
+    delete p;  // GOOD
 }