Merge pull request #354 from geoffw0/return-exception

CPP: Remove successor edges after calls to non-returning functions

cpp/ql/src/jsf/4.13 Functions/AV Rule 114.ql

@@ -18,13 +18,17 @@ import cpp
    programmer, we can flag it anyway, since this is arguably a bug.) */
 
 predicate functionsMissingReturnStmt(Function f, ControlFlowNode blame) {
-                        f.fromSource() and
-      exists(Type returnType |
-        returnType = f.getType().getUnderlyingType().getUnspecifiedType() and
-        not returnType instanceof VoidType and
-        not returnType instanceof TemplateParameter
-      ) and
-      exists(ReturnStmt s | f.getAPredecessor() = s | blame = s.getAPredecessor())}
+  f.fromSource() and
+  exists(Type returnType |
+    returnType = f.getType().getUnderlyingType().getUnspecifiedType() and
+    not returnType instanceof VoidType and
+    not returnType instanceof TemplateParameter
+  ) and
+  exists(ReturnStmt s |
+    f.getAPredecessor() = s and
+    blame = s.getAPredecessor()
+  )
+}
 
 /* If a function has a value-carrying return statement, but the extractor hit a snag
    whilst parsing the value, then the control flow graph will not include the value.

cpp/ql/src/semmle/code/cpp/controlflow/BasicBlocks.qll

@@ -240,13 +240,18 @@ class BasicBlock extends ControlFlowNodeBase {
 
   /**
    * Holds if control flow may reach this basic block from a function entry
-   * point or a `catch` clause of a reachable `try` statement.
+   * point or any handler of a reachable `try` statement.
    */
   predicate isReachable() {
     exists(Function f | f.getBlock() = this)
     or
-    exists(TryStmt t, BasicBlock tryblock | this = t.getACatchClause() and tryblock.isReachable() and tryblock.contains(t))
-    or
+    exists(TryStmt t, BasicBlock tryblock |
+      // a `Handler` preceeds the `CatchBlock`, and is always the beginning
+      // of a new `BasicBlock` (see `primitive_basic_block_entry_node`).
+      this.(Handler).getTryStmt() = t and
+      tryblock.isReachable() and
+      tryblock.contains(t)
+    ) or
     exists(BasicBlock pred | pred.getASuccessor() = this and pred.isReachable())
   }
 

cpp/ql/src/semmle/code/cpp/controlflow/ControlFlowGraph.qll

@@ -95,7 +95,7 @@ private cached module Cached {
     or
     reachable(n.getAPredecessor())
     or
-    n instanceof CatchBlock
+    n instanceof Handler
   }
 
   /** Holds if `condition` always evaluates to a nonzero value. */

cpp/ql/src/semmle/code/cpp/controlflow/internal/ConstantExprs.qll

@@ -136,6 +136,28 @@ private predicate impossibleDefaultSwitchEdge(Block switchBlock, DefaultCase dc)
                        val <= switchCaseRangeEnd(sc))))
 }
 
+/**
+ * Holds if the function `f` never returns due to not containing a return
+ * statement (explicit or compiler generated).  This can be thought of as
+ * a lightweight `potentiallyReturningFunction`- reachability of return
+ * statements is not checked.
+ */
+private predicate nonReturningFunction(Function f)
+{
+  exists(f.getBlock()) and
+  not exists(ReturnStmt ret | ret.getEnclosingFunction() = f) and
+  not getOptions().exits(f)
+}
+
+/**
+ * An edge from a call to a function that never returns is impossible.
+ */
+private predicate impossibleFunctionReturn(FunctionCall fc, Node succ) {
+  nonReturningFunction(fc.getTarget()) and
+  not fc.isVirtual() and
+  successors_extended(fc, succ)
+}
+
 /**
  * If `pred` is a function call with (at least) one function target,
  * (at least) one such target must be potentially returning.
@@ -158,6 +180,7 @@ cached predicate successors_adapted(Node pred, Node succ) {
   and not impossibleTrueEdge(pred, succ)
   and not impossibleSwitchEdge(pred, succ)
   and not impossibleDefaultSwitchEdge(pred, succ)
+  and not impossibleFunctionReturn(pred, succ)
   and not getOptions().exprExits(pred)
 }
 

cpp/ql/src/semmle/code/cpp/controlflow/internal/PrimitiveBasicBlocks.qll

@@ -44,6 +44,14 @@ private cached module Cached {
     // that the node have at least one successor.
     or
     (not successors_extended(_, node) and successors_extended(node, _))
+
+    // An exception handler is always the start of a new basic block. We
+    // don't generate edges for [possible] exceptions, but in practice control
+    // flow could reach the handler from anywhere inside the try block that
+    // could throw an exception of a corresponding type. A `Handler` usually
+    // needs to be considered reachable (see also `BasicBlock.isReachable`).
+    or
+    node instanceof Handler
   }
 
   /** Holds if `n2` follows `n1` in a `PrimitiveBasicBlock`. */