codeql: add query to check for wrong pool ele_acquire null check

pool ele_acquire can never return NULL (even when the pool is empty)
so checking for that is likely to be a misunderstanding or superflous.

Author: intrigus-lgtm

contrib/codeql/dev/MissingPoolFreeCheck.ql

@@ -10,34 +10,10 @@
  */
 
 import cpp
-
-class PoolAcquire extends FunctionCall {
-  PoolAcquire() {
-    (this.getTarget().getName().matches("%_idx_acquire") or this.getTarget().getName().matches("%_ele_acquire")) and
-    this.getTarget().getLocation().getFile().getBaseName().matches("fd_pool.c") and
-    not this.getLocation().getFile().getAbsolutePath().matches("%/tmpl/%")
-  }
-  string getPoolName() {
-      result = this.getTarget().getName().replaceAll("_idx_acquire", "").replaceAll("_ele_acquire", "")
-  }
-}
-
-
-class PoolFree extends FunctionCall {
-  PoolFree() {
-    (this.getTarget().getName().matches("%_free") or this.getTarget().getName().matches("%_used")) and
-    this.getTarget().getLocation().getFile().getBaseName().matches("fd_pool.c") and
-    not this.getLocation().getFile().getAbsolutePath().matches("%/tmpl/%")
-  }
-
-  string getPoolName() {
-    result = this.getTarget().getName().replaceAll("_free", "").replaceAll("_used", "")
-  }
-}
-
+import fdpool
 
 from PoolAcquire acq
 where
-not exists(PoolFree free | dominates(free, acq) and free.getPoolName() = acq.getPoolName()) and
-not acq.getLocation().getFile().getBaseName() = "fd_types.c"
+  not exists(PoolFree free | dominates(free, acq) and free.getPoolName() = acq.getPoolName()) and
+  not acq.getLocation().getFile().getBaseName() = "fd_types.c"
 select acq, acq.getPoolName()

contrib/codeql/dev/fdpool.qll

@@ -0,0 +1,29 @@
+import cpp
+
+class PoolAcquire extends FunctionCall {
+  PoolAcquire() {
+    (
+      this.getTarget().getName().matches("%_idx_acquire") or
+      this.getTarget().getName().matches("%_ele_acquire")
+    ) and
+    this.getTarget().getLocation().getFile().getBaseName().matches("fd_pool.c") and
+    not this.getLocation().getFile().getAbsolutePath().matches("%/tmpl/%")
+  }
+
+  string getPoolName() {
+    result =
+      this.getTarget().getName().replaceAll("_idx_acquire", "").replaceAll("_ele_acquire", "")
+  }
+}
+
+class PoolFree extends FunctionCall {
+  PoolFree() {
+    (this.getTarget().getName().matches("%_free") or this.getTarget().getName().matches("%_used")) and
+    this.getTarget().getLocation().getFile().getBaseName().matches("fd_pool.c") and
+    not this.getLocation().getFile().getAbsolutePath().matches("%/tmpl/%")
+  }
+
+  string getPoolName() {
+    result = this.getTarget().getName().replaceAll("_free", "").replaceAll("_used", "")
+  }
+}

contrib/codeql/nightly/WrongPoolAcquireCheck.ql

@@ -0,0 +1,42 @@
+/**
+ * @name Checking for null after pool ele_acquire is dubious.
+ * @description Pool ele_acquire functions never return null even when
+ * the pool is exhausted. Therefore, checking their return value for null
+ * indicates a misunderstanding and is likely a bug.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id asymmetric-research/fd-pool-ele-acquire-wrong-null-check
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.new.DataFlow
+import fdpool
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.controlflow.IRGuards
+
+module AcquireToNullCheck implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(PoolAcquire pa | pa.isEleAcquire() | source.asExpr() = pa)
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(IRGuardCondition g |
+      g.comparesEq(sink.asInstruction().getAUse(),
+        any(ConstantValueInstruction const | const.getValue() = "0").getAUse(), 0, false, _)
+    )
+  }
+}
+
+module Flow = DataFlow::Global<AcquireToNullCheck>;
+
+import Flow::PathGraph
+
+from Flow::PathNode sourceNode, Flow::PathNode sinkNode
+where
+  // only intra-procedural flow to reduce FPs
+  sourceNode.getNode().getEnclosingCallable() = sinkNode.getNode().getEnclosingCallable() and
+  Flow::flowPath(sourceNode, sinkNode)
+select sinkNode, sourceNode, sinkNode,
+  "Result of $@ cannot be null; checking it indicates a misunderstanding.", sourceNode,
+  "pool ele_acquire"

contrib/codeql/nightly/qlpack.yml

@@ -4,3 +4,4 @@ extractor: cpp
 warnOnImplicitThis: true
 dependencies:
   codeql/cpp-all: "*"
+  asymmetric-research/fd-dev-queries: "*"