C++: Fix FP and accept test changes.

Author: MathiasVP

cpp/ql/src/Critical/DoubleFree.ql

@@ -18,19 +18,6 @@ import DoubleFree::PathGraph
 
 predicate isFree(DataFlow::Node n, Expr e) { isFree(n, e, _) }
 
-/**
- * Holds if `fc` is a function call that is the result of expanding
- * the `ExFreePool` macro.
- */
-predicate isExFreePoolCall(FunctionCall fc) {
-  exists(MacroInvocation mi |
-    mi.getMacroName() = "ExFreePool" and
-    mi.getExpr() = fc
-  )
-  or
-  fc.getTarget().hasGlobalName("ExFreePool")
-}
-
 /**
  * `dealloc1` is a deallocation expression and `e` is an expression such
  * that is deallocated by a deallocation expression, and the `(dealloc1, e)` pair
@@ -46,7 +33,7 @@ predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) {
     // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl:
     // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool
     // to release the memory that was allocated for the MDL structure."
-    isExFreePoolCall(dealloc2)
+    isExFreePoolCall(dealloc2, _)
   )
 }
 

cpp/ql/src/Critical/FlowAfterFree.qll

@@ -111,3 +111,19 @@ predicate isFree(DataFlow::Node n, Expr e, DeallocationExpr dealloc) {
   // Ignore realloc functions
   not exists(dealloc.(FunctionCall).getTarget().(AllocationFunction).getReallocPtrArg())
 }
+
+/**
+ * Holds if `fc` is a function call that is the result of expanding
+ * the `ExFreePool` macro.
+ */
+predicate isExFreePoolCall(FunctionCall fc, Expr e) {
+  e = fc.getArgument(0) and
+  (
+    exists(MacroInvocation mi |
+      mi.getMacroName() = "ExFreePool" and
+      mi.getExpr() = fc
+    )
+    or
+    fc.getTarget().hasGlobalName("ExFreePool")
+  )
+}

cpp/ql/src/Critical/UseAfterFree.ql

@@ -146,9 +146,20 @@ predicate isUse(DataFlow::Node n, Expr e) {
   )
 }
 
-predicate excludeNothing(DeallocationExpr dealloc, Expr e) { none() }
+/**
+ * `dealloc1` is a deallocation expression, `e` is an expression that dereferences a
+ * pointer, and the `(dealloc1, e)` pair should be excluded by the `FlowFromFree` library.
+ */
+bindingset[dealloc1, e]
+predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) {
+  // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl:
+  // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool
+  // to release the memory that was allocated for the MDL structure."
+  dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and
+  isExFreePoolCall(_, e)
+}
 
-module UseAfterFree = FlowFromFree<isUse/2, excludeNothing/2>;
+module UseAfterFree = FlowFromFree<isUse/2, isExcludeFreeUsePair/2>;
 
 from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc
 where

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

@@ -15,7 +15,6 @@ edges
 | 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:152:27:152:27 | a | test_free.cpp:153:5:153:5 | a |
-| test_free.cpp:227:24:227:45 | memory_descriptor_list | test_free.cpp:228:16:228:37 | memory_descriptor_list |
 nodes
 | test_free.cpp:11:10:11:10 | a | semmle.label | a |
 | test_free.cpp:11:10:11:10 | a | semmle.label | a |
@@ -40,8 +39,6 @@ nodes
 | test_free.cpp:152:27:152:27 | a | semmle.label | a |
 | test_free.cpp:152:27:152:27 | a | semmle.label | a |
 | test_free.cpp:153:5:153:5 | a | semmle.label | a |
-| test_free.cpp:227:24:227:45 | memory_descriptor_list | semmle.label | memory_descriptor_list |
-| test_free.cpp:228:16:228:37 | memory_descriptor_list | semmle.label | memory_descriptor_list |
 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 |
@@ -60,4 +57,3 @@ subpaths
 | 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: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:228:16:228:37 | memory_descriptor_list | test_free.cpp:227:24:227:45 | memory_descriptor_list | test_free.cpp:228:16:228:37 | memory_descriptor_list | Memory may have been previously freed by $@. | test_free.cpp:227:5:227:22 | call to MmFreePagesFromMdl | call to MmFreePagesFromMdl |

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

@@ -225,5 +225,5 @@ void MmFreePagesFromMdl(void*);
 void ExFreePool(void*);
 void test_ms_free(void * memory_descriptor_list) {
     MmFreePagesFromMdl(memory_descriptor_list); //GOOD
-    ExFreePool(memory_descriptor_list); // GOOD [FALSE POSITIVE for cpp/use-after-free]
+    ExFreePool(memory_descriptor_list); // GOOD
 }