C++: Use Guards library in Overflow.qll

Replaces the ad-hoc guard handling with the Guards library. Fixes an
observed false positive pattern, and (hopefully) means some pragmas are
no longer necessary for performance.

Author: rdmarsh2

cpp/ql/lib/semmle/code/cpp/security/Overflow.qll

@@ -5,64 +5,27 @@
 
 import cpp
 import semmle.code.cpp.controlflow.Dominance
-// `GlobalValueNumbering` is only imported to prevent IR re-evaluation.
 private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
+import semmle.code.cpp.controlflow.Guards
 
 /**
  * Holds if the value of `use` is guarded using `abs`.
  */
 predicate guardedAbs(Operation e, Expr use) {
   exists(FunctionCall fc | fc.getTarget().getName() = ["abs", "labs", "llabs", "imaxabs"] |
     fc.getArgument(0).getAChild*() = use and
-    guardedLesser(e, fc)
+    exists(GuardCondition c | c.ensuresLt(use, _, _, e.getBasicBlock(), true))
   )
 }
 
-/**
- * Gets the position of `stmt` in basic block `block` (this is a thin layer
- * over `BasicBlock.getNode`, intended to improve performance).
- */
-pragma[noinline]
-private int getStmtIndexInBlock(BasicBlock block, Stmt stmt) { block.getNode(result) = stmt }
-
-pragma[inline]
-private predicate stmtDominates(Stmt dominator, Stmt dominated) {
-  // In same block
-  exists(BasicBlock block, int dominatorIndex, int dominatedIndex |
-    dominatorIndex = getStmtIndexInBlock(block, dominator) and
-    dominatedIndex = getStmtIndexInBlock(block, dominated) and
-    dominatedIndex >= dominatorIndex
-  )
-  or
-  // In (possibly) different blocks
-  bbStrictlyDominates(dominator.getBasicBlock(), dominated.getBasicBlock())
-}
-
 /**
  * Holds if the value of `use` is guarded to be less than something, and `e`
  * is in code controlled by that guard (where the guard condition held).
  */
-pragma[nomagic]
 predicate guardedLesser(Operation e, Expr use) {
-  exists(IfStmt c, RelationalOperation guard |
-    use = guard.getLesserOperand().getAChild*() and
-    guard = c.getControllingExpr().getAChild*() and
-    stmtDominates(c.getThen(), e.getEnclosingStmt())
-  )
-  or
-  exists(Loop c, RelationalOperation guard |
-    use = guard.getLesserOperand().getAChild*() and
-    guard = c.getControllingExpr().getAChild*() and
-    stmtDominates(c.getStmt(), e.getEnclosingStmt())
-  )
-  or
-  exists(ConditionalExpr c, RelationalOperation guard |
-    use = guard.getLesserOperand().getAChild*() and
-    guard = c.getCondition().getAChild*() and
-    c.getThen().getAChild*() = e
-  )
+  exists(GuardCondition c | c.ensuresLt(use, _, _, e.getBasicBlock(), true))
   or
   guardedAbs(e, use)
 }
@@ -71,25 +34,8 @@ predicate guardedLesser(Operation e, Expr use) {
  * Holds if the value of `use` is guarded to be greater than something, and `e`
  * is in code controlled by that guard (where the guard condition held).
  */
-pragma[nomagic]
 predicate guardedGreater(Operation e, Expr use) {
-  exists(IfStmt c, RelationalOperation guard |
-    use = guard.getGreaterOperand().getAChild*() and
-    guard = c.getControllingExpr().getAChild*() and
-    stmtDominates(c.getThen(), e.getEnclosingStmt())
-  )
-  or
-  exists(Loop c, RelationalOperation guard |
-    use = guard.getGreaterOperand().getAChild*() and
-    guard = c.getControllingExpr().getAChild*() and
-    stmtDominates(c.getStmt(), e.getEnclosingStmt())
-  )
-  or
-  exists(ConditionalExpr c, RelationalOperation guard |
-    use = guard.getGreaterOperand().getAChild*() and
-    guard = c.getCondition().getAChild*() and
-    c.getThen().getAChild*() = e
-  )
+  exists(GuardCondition c | c.ensuresLt(use, _, _, e.getBasicBlock(), false))
   or
   guardedAbs(e, use)
 }

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ArithmeticUncontrolled/ArithmeticUncontrolled.expected

@@ -11,8 +11,6 @@ edges
 | test.c:137:13:137:16 | call to rand | test.c:139:10:139:10 | r |
 | test.c:155:22:155:25 | call to rand | test.c:157:9:157:9 | r |
 | test.c:155:22:155:27 | (unsigned int)... | test.c:157:9:157:9 | r |
-| test.c:162:19:162:38 | (unsigned int)... | test.c:166:16:166:19 | data |
-| test.c:162:33:162:36 | call to rand | test.c:166:16:166:19 | data |
 | test.cpp:6:5:6:12 | ReturnValue | test.cpp:24:11:24:18 | call to get_rand |
 | test.cpp:8:9:8:12 | call to rand | test.cpp:6:5:6:12 | ReturnValue |
 | test.cpp:13:2:13:6 | * ... [post update] | test.cpp:30:13:30:14 | & ... [post update] |
@@ -59,9 +57,6 @@ nodes
 | test.c:155:22:155:25 | call to rand | semmle.label | call to rand |
 | test.c:155:22:155:27 | (unsigned int)... | semmle.label | (unsigned int)... |
 | test.c:157:9:157:9 | r | semmle.label | r |
-| test.c:162:19:162:38 | (unsigned int)... | semmle.label | (unsigned int)... |
-| test.c:162:33:162:36 | call to rand | semmle.label | call to rand |
-| test.c:166:16:166:19 | data | semmle.label | data |
 | test.cpp:6:5:6:12 | ReturnValue | semmle.label | ReturnValue |
 | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand |
 | test.cpp:13:2:13:6 | * ... [post update] | semmle.label | * ... [post update] |
@@ -109,8 +104,6 @@ subpaths
 | test.c:139:10:139:10 | r | test.c:137:13:137:16 | call to rand | test.c:139:10:139:10 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:137:13:137:16 | call to rand | Uncontrolled value |
 | test.c:157:9:157:9 | r | test.c:155:22:155:25 | call to rand | test.c:157:9:157:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:155:22:155:25 | call to rand | Uncontrolled value |
 | test.c:157:9:157:9 | r | test.c:155:22:155:27 | (unsigned int)... | test.c:157:9:157:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:155:22:155:25 | call to rand | Uncontrolled value |
-| test.c:166:16:166:19 | data | test.c:162:19:162:38 | (unsigned int)... | test.c:166:16:166:19 | data | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:162:33:162:36 | call to rand | Uncontrolled value |
-| test.c:166:16:166:19 | data | test.c:162:33:162:36 | call to rand | test.c:166:16:166:19 | data | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:162:33:162:36 | call to rand | Uncontrolled value |
 | test.cpp:25:7:25:7 | r | test.cpp:8:9:8:12 | call to rand | test.cpp:25:7:25:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:8:9:8:12 | call to rand | Uncontrolled value |
 | test.cpp:31:7:31:7 | r | test.cpp:13:10:13:13 | call to rand | test.cpp:31:7:31:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:13:10:13:13 | call to rand | Uncontrolled value |
 | test.cpp:37:7:37:7 | r | test.cpp:18:9:18:12 | call to rand | test.cpp:37:7:37:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:18:9:18:12 | call to rand | Uncontrolled value |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ArithmeticUncontrolled/test.c

@@ -163,5 +163,5 @@ void guarded_test(unsigned p) {
   if (p >= data) {
     return;
   }
-  unsigned z = data - p; // GOOD [FALSE POSITIVE]
+  unsigned z = data - p; // GOOD
 }