C++: Improve predicate upperBound in SimpleRangeAnalysis

If an expression has an immediate guardPhi node, this is used as a strict upper bound

Author: andersfugmann

cpp/change-notes/2021-08-31-range-analysis-upper-bound.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* The `SimpleRangeAnalysis` library includes information from the
+  immediate guard for determining the upper bound of a stack
+  variable for improved accuracy.

cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeSSA.qll

@@ -95,10 +95,19 @@ class RangeSsaDefinition extends ControlFlowNodeBase {
 
   /**
    * If this definition is a phi node corresponding to a guard,
-   * then return the variable and the guard.
+   * then return the variable access and the guard.
    */
-  predicate isGuardPhi(VariableAccess v, Expr guard, boolean branch) {
-    guard_defn(v, guard, this, branch)
+  predicate isGuardPhi(VariableAccess va, Expr guard, boolean branch) {
+    guard_defn(va, guard, this, branch)
+  }
+
+  /**
+   * If this definition is a phi node corresponding to a guard,
+   * then return the variable guarded, the variable access and the guard.
+   */
+  predicate isGuardPhi(StackVariable v, VariableAccess va, Expr guard, boolean branch) {
+    guard_defn(va, guard, this, branch) and
+    va.getTarget() = v
   }
 
   /** Gets the primary location of this definition. */

cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -1530,6 +1530,22 @@ private predicate isUnsupportedGuardPhi(Variable v, RangeSsaDefinition phi, Vari
   )
 }
 
+/**
+ * Gets the upper bound of the expression, if the expression is guarded.
+ * An upper bound can only be found, if a guard phi node can be found, and the
+ * expression has only one immediate predecessor.
+ */
+private float getGuardedUpperBound(VariableAccess guardedAccess) {
+  exists(
+    RangeSsaDefinition def, StackVariable v, VariableAccess guardVa, Expr guard, boolean branch
+  |
+    def.isGuardPhi(v, guardVa, guard, branch) and
+    exists(unique(BasicBlock b | b = def.(BasicBlock).getAPredecessor())) and
+    guardedAccess = def.getAUse(v) and
+    result = max(float ub | upperBoundFromGuard(guard, guardVa, ub, branch))
+  )
+}
+
 cached
 private module SimpleRangeAnalysisCached {
   /**
@@ -1565,9 +1581,9 @@ private module SimpleRangeAnalysisCached {
    */
   cached
   float upperBound(Expr expr) {
-    // Combine the upper bounds returned by getTruncatedUpperBounds into a
-    // single maximum value.
-    result = max(float ub | ub = getTruncatedUpperBounds(expr) | ub)
+    // Combine the upper bounds returned by getTruncatedUpperBounds and
+    // getGuardedUpperBound into a single maximum value
+    result = min([max(getTruncatedUpperBounds(expr)), getGuardedUpperBound(expr)])
   }
 
   /**

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.expected

@@ -584,9 +584,9 @@
 | test.c:639:9:639:10 | ss | 2 |
 | test.c:645:8:645:8 | s | 2147483647 |
 | test.c:645:15:645:15 | s | 127 |
-| test.c:645:23:645:23 | s | 15 |
-| test.c:646:18:646:18 | s | 15 |
-| test.c:646:22:646:22 | s | 15 |
+| test.c:645:23:645:23 | s | 9 |
+| test.c:646:18:646:18 | s | 9 |
+| test.c:646:22:646:22 | s | 9 |
 | test.c:647:9:647:14 | result | 127 |
 | test.c:653:7:653:7 | i | 0 |
 | test.c:654:9:654:9 | i | 2147483647 |
@@ -650,7 +650,7 @@
 | test.cpp:97:10:97:10 | i | 65535 |
 | test.cpp:97:22:97:22 | i | 32767 |
 | test.cpp:98:5:98:5 | i | 2147483647 |
-| test.cpp:98:9:98:9 | i | 32767 |
+| test.cpp:98:9:98:9 | i | 12345 |
 | test.cpp:99:5:99:5 | i | 32767 |
 | test.cpp:106:7:106:7 | n | 32767 |
 | test.cpp:109:7:109:7 | n | 32767 |

cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected

@@ -14,4 +14,3 @@
 | test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. |
 | test.cpp:26:27:26:27 | 4 | Potential buffer-overflow: 'buffer2' has size 3 not 4. |
 | test.cpp:40:22:40:27 | amount | Potential buffer-overflow: 'buffer' has size 100 not 101. |
-| test.cpp:55:13:55:21 | access to array | Potential buffer-overflow: counter 'i' <= 9 but 'buffer' has 5 elements. |