Merge pull request github#6568 from andersfugmann/andersfugmann/improve_upper_bound

C++: Improve predicate upperBound in SimpleRangeAnalysis

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/lib/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/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -1530,6 +1530,29 @@ 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
+    // If the basic block for the variable access being examined has
+    // more than one predecessor, the guard phi node could originate
+    // from one of the predecessors. This is because the guard phi
+    // node is attached to the block at the end of the edge and not on
+    // the actual edge. It is therefore not possible to determine which
+    // edge the guard phi node belongs to. The predicate below ensures
+    // that there is one predecessor, albeit somewhat conservative.
+    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 +1588,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/lowerBound.expected

@@ -594,6 +594,11 @@
 | test.c:659:9:659:9 | u | 0 |
 | test.c:664:12:664:12 | s | -2147483648 |
 | test.c:665:7:665:8 | s2 | -4 |
+| test.c:670:7:670:7 | x | -2147483648 |
+| test.c:671:9:671:9 | y | -2147483648 |
+| test.c:675:7:675:7 | y | -2147483648 |
+| test.c:684:7:684:7 | x | -2147483648 |
+| test.c:689:7:689:7 | x | -2147483648 |
 | test.cpp:10:7:10:7 | b | -2147483648 |
 | test.cpp:11:5:11:5 | x | -2147483648 |
 | test.cpp:13:10:13:10 | x | -2147483648 |
@@ -647,16 +652,18 @@
 | test.cpp:97:10:97:10 | i | -2147483648 |
 | test.cpp:97:22:97:22 | i | -2147483648 |
 | test.cpp:98:5:98:5 | i | -2147483648 |
-| test.cpp:105:7:105:7 | n | -32768 |
-| test.cpp:108:7:108:7 | n | 0 |
-| test.cpp:109:5:109:5 | n | 1 |
-| test.cpp:111:5:111:5 | n | 0 |
-| test.cpp:114:8:114:8 | n | 0 |
-| test.cpp:115:5:115:5 | n | 0 |
-| test.cpp:117:5:117:5 | n | 1 |
-| test.cpp:120:3:120:3 | n | 0 |
-| test.cpp:120:8:120:8 | n | 1 |
-| test.cpp:120:12:120:12 | n | 0 |
-| test.cpp:121:4:121:4 | n | 0 |
-| test.cpp:121:8:121:8 | n | 0 |
-| test.cpp:121:12:121:12 | n | 1 |
+| test.cpp:98:9:98:9 | i | -2147483648 |
+| test.cpp:99:5:99:5 | i | -2147483648 |
+| test.cpp:106:7:106:7 | n | -32768 |
+| test.cpp:109:7:109:7 | n | 0 |
+| test.cpp:110:5:110:5 | n | 1 |
+| test.cpp:112:5:112:5 | n | 0 |
+| test.cpp:115:8:115:8 | n | 0 |
+| test.cpp:116:5:116:5 | n | 0 |
+| test.cpp:118:5:118:5 | n | 1 |
+| test.cpp:121:3:121:3 | n | 0 |
+| test.cpp:121:8:121:8 | n | 1 |
+| test.cpp:121:12:121:12 | n | 0 |
+| test.cpp:122:4:122:4 | n | 0 |
+| test.cpp:122:8:122:8 | n | 0 |
+| test.cpp:122:12:122:12 | n | 1 |

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

@@ -15,5 +15,5 @@
 | test.c:394:20:394:36 | ... ? ... : ... | 0.0 | 0.0 | 100.0 |
 | test.c:606:5:606:14 | ... ? ... : ... | 0.0 | 1.0 | 0.0 |
 | test.c:607:5:607:14 | ... ? ... : ... | 0.0 | 0.0 | 1.0 |
-| test.cpp:120:3:120:12 | ... ? ... : ... | 0.0 | 1.0 | 0.0 |
-| test.cpp:121:3:121:12 | ... ? ... : ... | 0.0 | 0.0 | 1.0 |
+| test.cpp:121:3:121:12 | ... ? ... : ... | 0.0 | 1.0 | 0.0 |
+| test.cpp:122:3:122:12 | ... ? ... : ... | 0.0 | 0.0 | 1.0 |

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

@@ -15,5 +15,5 @@
 | test.c:394:20:394:36 | ... ? ... : ... | 100.0 | 99.0 | 100.0 |
 | test.c:606:5:606:14 | ... ? ... : ... | 32767.0 | 32767.0 | 0.0 |
 | test.c:607:5:607:14 | ... ? ... : ... | 32767.0 | 0.0 | 32767.0 |
-| test.cpp:120:3:120:12 | ... ? ... : ... | 32767.0 | 32767.0 | 0.0 |
-| test.cpp:121:3:121:12 | ... ? ... : ... | 32767.0 | 0.0 | 32767.0 |
+| test.cpp:121:3:121:12 | ... ? ... : ... | 32767.0 | 32767.0 | 0.0 |
+| test.cpp:122:3:122:12 | ... ? ... : ... | 32767.0 | 0.0 | 32767.0 |

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c

@@ -664,3 +664,28 @@ void test_mod(int s) {
   int s2 = s % 5;
   out(s2); // -4 .. 4
 }
+
+void exit(int);
+void guard_with_exit(int x, int y) {
+  if (x) {
+    if (y != 0) {
+      exit(0);
+    }
+  }
+  out(y); // ..
+
+  // This test ensures that we correctly identify
+  // that the upper bound for y is max_int when calling `out(y)`.
+  // The RangeSsa will place guardPhy on `out(y)`, and consequently there is no
+  // frontier phi node at out(y).
+}
+
+void test(int x) {
+  if (x >= 10) {
+    return;
+  }
+  // The basic below has two predecessors.
+label:
+  out(x);
+  goto label;
+}

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

@@ -95,6 +95,7 @@ int ref_to_number(int &i, const int &ci, int &aliased) {
     return alias;
 
   for (; i <= 12345; i++) { // test that widening works for references
+    i = i;
     i;
   }
 

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

@@ -584,16 +584,21 @@
 | 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 |
 | test.c:658:7:658:7 | u | 0 |
 | test.c:659:9:659:9 | u | 4294967295 |
 | test.c:664:12:664:12 | s | 2147483647 |
 | test.c:665:7:665:8 | s2 | 4 |
+| test.c:670:7:670:7 | x | 2147483647 |
+| test.c:671:9:671:9 | y | 2147483647 |
+| test.c:675:7:675:7 | y | 2147483647 |
+| test.c:684:7:684:7 | x | 2147483647 |
+| test.c:689:7:689:7 | x | 15 |
 | test.cpp:10:7:10:7 | b | 2147483647 |
 | test.cpp:11:5:11:5 | x | 2147483647 |
 | test.cpp:13:10:13:10 | x | 2147483647 |
@@ -646,17 +651,19 @@
 | test.cpp:95:12:95:16 | alias | 2147483647 |
 | test.cpp:97:10:97:10 | i | 65535 |
 | test.cpp:97:22:97:22 | i | 32767 |
-| test.cpp:98:5:98:5 | i | 32767 |
-| test.cpp:105:7:105:7 | n | 32767 |
-| test.cpp:108:7:108:7 | n | 32767 |
-| test.cpp:109:5:109:5 | n | 32767 |
-| test.cpp:111:5:111:5 | n | 0 |
-| test.cpp:114:8:114:8 | n | 32767 |
-| test.cpp:115:5:115:5 | n | 0 |
-| test.cpp:117:5:117:5 | n | 32767 |
-| test.cpp:120:3:120:3 | n | 32767 |
-| test.cpp:120:8:120:8 | n | 32767 |
-| test.cpp:120:12:120:12 | n | 0 |
-| test.cpp:121:4:121:4 | n | 32767 |
-| test.cpp:121:8:121:8 | n | 0 |
-| test.cpp:121:12:121:12 | n | 32767 |
+| test.cpp:98:5:98:5 | i | 2147483647 |
+| 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 |
+| test.cpp:110:5:110:5 | n | 32767 |
+| test.cpp:112:5:112:5 | n | 0 |
+| test.cpp:115:8:115:8 | n | 32767 |
+| test.cpp:116:5:116:5 | n | 0 |
+| test.cpp:118:5:118:5 | n | 32767 |
+| test.cpp:121:3:121:3 | n | 32767 |
+| test.cpp:121:8:121:8 | n | 32767 |
+| test.cpp:121:12:121:12 | n | 0 |
+| test.cpp:122:4:122:4 | n | 32767 |
+| test.cpp:122:8:122:8 | n | 0 |
+| test.cpp:122:12:122:12 | n | 32767 |

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

@@ -46,3 +46,13 @@ void f2(char *src)
 	ptr = &(buffer[1]);
 	memcpy(ptr, src, 100); // BAD [NOT DETECTED]
 }
+
+void f3() {
+    int i;
+    char buffer[5];
+    for (i=0; i<10; i++) {
+        if (i < 5) {
+            buffer[i] = 0; // GOOD
+        }
+    }
+}