Updating FlowAfterFree to not enforce dominance of source/sink. DoubleFree and UseAfterFree queries now enforce dominance.

Author: bdrodes

cpp/ql/src/Critical/DoubleFree.ql

@@ -13,6 +13,7 @@
 
 import cpp
 import semmle.code.cpp.dataflow.new.DataFlow
+import semmle.code.cpp.ir.IR
 import FlowAfterFree
 import DoubleFree::PathGraph
 
@@ -43,11 +44,26 @@ predicate isExcludeFreePair(DeallocationExpr dealloc1, Expr e) {
 
 module DoubleFree = FlowFromFree<isFree/2, isExcludeFreePair/2>;
 
-from DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2
+/*
+ * In order to reduce false positives, the set of sinks is restricted to only those
+ * that satisfy at least one of the following two criteria:
+ * 1. The source dominates the sink, or
+ * 2. The sink post-dominates the source.
+ */
+
+from
+  DoubleFree::PathNode source, DoubleFree::PathNode sink, DeallocationExpr dealloc, Expr e2,
+  DataFlow::Node srcNode, DataFlow::Node sinkNode
 where
   DoubleFree::flowPath(source, sink) and
-  isFree(source.getNode(), _, _, dealloc) and
-  isFree(sink.getNode(), e2)
-select sink.getNode(), source, sink,
+  source.getNode() = srcNode and
+  sink.getNode() = sinkNode and
+  isFree(srcNode, _, _, dealloc) and
+  isFree(sinkNode, e2) and
+  (
+    sinkStrictlyPostDominatesSource(srcNode, sinkNode) or
+    sourceStrictlyDominatesSink(srcNode, sinkNode)
+  )
+select sinkNode, source, sink,
   "Memory pointed to by '" + e2.toString() + "' may already have been freed by $@.", dealloc,
   dealloc.toString()

cpp/ql/src/Critical/FlowAfterFree.qll

@@ -38,14 +38,25 @@ predicate strictlyDominates(IRBlock b1, int i1, IRBlock b2, int i2) {
   b1.strictlyDominates(b2)
 }
 
+predicate sinkStrictlyPostDominatesSource(DataFlow::Node source, DataFlow::Node sink) {
+  exists(IRBlock b1, int i1, IRBlock b2, int i2 |
+    source.hasIndexInBlock(b1, i1) and
+    sink.hasIndexInBlock(b2, i2) and
+    strictlyPostDominates(b2, i2, b1, i1)
+  )
+}
+
+predicate sourceStrictlyDominatesSink(DataFlow::Node source, DataFlow::Node sink) {
+  exists(IRBlock b1, int i1, IRBlock b2, int i2 |
+    source.hasIndexInBlock(b1, i1) and
+    sink.hasIndexInBlock(b2, i2) and
+    strictlyDominates(b1, i1, b2, i2)
+  )
+}
+
 /**
  * Constructs a `FlowFromFreeConfig` module that can be used to find flow between
  * a pointer being freed by some deallocation function, and a user-specified sink.
- *
- * In order to reduce false positives, the set of sinks is restricted to only those
- * that satisfy at least one of the following two criteria:
- * 1. The source dominates the sink, or
- * 2. The sink post-dominates the source.
  */
 module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
   module FlowFromFreeConfig implements DataFlow::StateConfigSig {
@@ -59,20 +70,11 @@ module FlowFromFree<isSinkSig/2 isASink, isExcludedSig/2 isExcluded> {
 
     pragma[inline]
     predicate isSink(DataFlow::Node sink, FlowState state) {
-      exists(
-        Expr e, DataFlow::Node source, IRBlock b1, int i1, IRBlock b2, int i2,
-        DeallocationExpr dealloc
-      |
+      exists(Expr e, DeallocationExpr dealloc |
         isASink(sink, e) and
-        isFree(source, _, state, dealloc) and
+        isFree(_, _, state, dealloc) and
         e != state and
-        source.hasIndexInBlock(b1, i1) and
-        sink.hasIndexInBlock(b2, i2) and
         not isExcluded(dealloc, e)
-      |
-        strictlyDominates(b1, i1, b2, i2)
-        or
-        strictlyPostDominates(b2, i2, b1, i1)
       )
     }
 

cpp/ql/src/Critical/UseAfterFree.ql

@@ -175,9 +175,24 @@ predicate isExcludeFreeUsePair(DeallocationExpr dealloc1, Expr e) {
 
 module UseAfterFree = FlowFromFree<isUse/2, isExcludeFreeUsePair/2>;
 
-from UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc
+/*
+ * In order to reduce false positives, the set of sinks is restricted to only those
+ * that satisfy at least one of the following two criteria:
+ * 1. The source dominates the sink, or
+ * 2. The sink post-dominates the source.
+ */
+
+from
+  UseAfterFree::PathNode source, UseAfterFree::PathNode sink, DeallocationExpr dealloc,
+  DataFlow::Node srcNode, DataFlow::Node sinkNode
 where
   UseAfterFree::flowPath(source, sink) and
-  isFree(source.getNode(), _, _, dealloc)
-select sink.getNode(), source, sink, "Memory may have been previously freed by $@.", dealloc,
+  source.getNode() = srcNode and
+  sink.getNode() = sinkNode and
+  isFree(srcNode, _, _, dealloc) and
+  (
+    sinkStrictlyPostDominatesSource(srcNode, sinkNode) or
+    sourceStrictlyDominatesSink(srcNode, sinkNode)
+  )
+select sinkNode, source, sink, "Memory may have been previously freed by $@.", dealloc,
   dealloc.toString()