C++: Respond to review comments.

Author: MathiasVP

cpp/ql/src/Critical/DoubleFree.ql

@@ -16,10 +16,11 @@ import semmle.code.cpp.dataflow.new.DataFlow
 import FlowAfterFree
 import DoubleFree::PathGraph
 
-predicate isFree(DataFlow::Node n, Expr e) {
-  n.asExpr() = e and
-  isFree(_, e, _)
-}
+/**
+ * Holds if `n` is a dataflow node that represents a pointer going into a
+ * deallocation function, and `e` is the corresponding expression.
+ */
+predicate isFree(DataFlow::Node n, Expr e) { isFree(_, n, e, _) }
 
 /**
  * `dealloc1` is a deallocation expression and `e` is an expression such
@@ -31,7 +32,7 @@ predicate isFree(DataFlow::Node n, Expr e) {
  */
 bindingset[dealloc1, e]
 predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) {
-  exists(DeallocationExpr dealloc2 | isFree(_, e, dealloc2) |
+  exists(DeallocationExpr dealloc2 | isFree(_, _, e, dealloc2) |
     dealloc1.(FunctionCall).getTarget().hasGlobalName("MmFreePagesFromMdl") and
     // From https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmfreepagesfrommdl:
     // "After calling MmFreePagesFromMdl, the caller must also call ExFreePool
@@ -45,7 +46,7 @@ module DoubleFree = FlowFromFree<isFree/2, isExcludeFreePair/2>;
 from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2
 where
   DoubleFree::flowPath(source, sink) and
-  isFree(source.getNode(), _, dealloc) and
+  isFree(source.getNode(), _, _, dealloc) and
   isFree(sink.getNode(), e2)
 select sink.getNode(), source, sink,
   "Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc,

cpp/ql/src/Critical/FlowAfterFree.qll

@@ -50,12 +50,12 @@ predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) {
 module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
   module FlowFromFreeConfig implements DataFlow::StateConfigSig {
     class FlowState instanceof Expr {
-      FlowState() { isFree(_, this, _) }
+      FlowState() { isFree(_, _, this, _) }
 
       string toString() { result = super.toString() }
     }
 
-    predicate isSource(DataFlow::Node node, FlowState state) { isFree(node, state, _) }
+    predicate isSource(DataFlow::Node node, FlowState state) { isFree(node, _, state, _) }
 
     pragma[inline]
     predicate isSink(DataFlow::Node sink, FlowState state) {
@@ -64,7 +64,7 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
         DeallocationExpr dealloc
       |
         isASink(sink, e) and
-        isFree(source, state, dealloc) and
+        isFree(source, _, state, dealloc) and
         e != state and
         source.hasIndexInBlock(b1, i1) and
         sink.hasIndexInBlock(b2, i2) and
@@ -98,11 +98,12 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
  * `dealloc` after the call returns (i.e., the post-update node associated with
  * the argument to `dealloc`).
  */
-predicate isFree(DataFlow::Node n, Expr e, DeallocationExpr dealloc) {
+predicate isFree(DataFlow::Node outgoing, DataFlow::Node incoming, Expr e, DeallocationExpr dealloc) {
   exists(Expr conv |
     e = conv.getUnconverted() and
     conv = dealloc.getFreedExpr().getFullyConverted() and
-    conv = n.(DataFlow::PostUpdateNode).getPreUpdateNode().asConvertedExpr()
+    incoming = outgoing.(DataFlow::PostUpdateNode).getPreUpdateNode() and
+    conv = incoming.asConvertedExpr()
   ) and
   // Ignore realloc functions
   not exists(dealloc.(FunctionCall).getTarget().(AllocationFunction).getReallocPtrArg())

cpp/ql/src/Critical/UseAfterFree.ql

@@ -30,7 +30,7 @@ private predicate externalCallNeverDereferences(FormattingFunctionCall call, int
 }
 
 predicate isUse0(Expr e) {
-  not isFree(_, e, _) and
+  not isFree(_, _, e, _) and
   (
     e = any(PointerDereferenceExpr pde).getOperand()
     or
@@ -170,6 +170,6 @@ module UseAfterFree = FlowFromFree<isUse/2, isExcludeFreeUsePair/2>;
 from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc
 where
   UseAfterFree::flowPath(source, sink) and
-  isFree(source.getNode(), _, dealloc)
+  isFree(source.getNode(), _, _, dealloc)
 select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc,
   dealloc.toString()