Merge pull request #4420 from jbj/SimpleRangeAnalysis-widen-Expr

C++: SimpleRangeAnalysis: widen recursive *, +, -

Author: geoffw0

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

@@ -460,6 +460,39 @@ private predicate isRecursiveDef(RangeSsaDefinition def, StackVariable v) {
   defDependsOnDefTransitively(def, v, def, v)
 }
 
+/**
+ * Holds if the bounds of `e` depend on a recursive definition, meaning that
+ * `e` is likely to have many candidate bounds during the main recursion.
+ */
+private predicate isRecursiveExpr(Expr e) {
+  exists(RangeSsaDefinition def, StackVariable v | exprDependsOnDef(e, def, v) |
+    isRecursiveDef(def, v)
+  )
+}
+
+/**
+ * Holds if `binop` is a binary operation that's likely to be assigned a
+ * quadratic (or more) number of candidate bounds during the analysis. This can
+ * happen when two conditions are satisfied:
+ * 1. It is likely there are many more candidate bounds for `binop` than for
+ *    its operands. For example, the number of candidate bounds for `x + y`,
+ *    denoted here nbounds(`x + y`), will be O(nbounds(`x`) * nbounds(`y`)).
+ *    In contrast, nbounds(`b ? x : y`) is only O(nbounds(`x`) + nbounds(`y`)).
+ * 2. Both operands of `binop` are recursively determined and are therefore
+ *    likely to have a large number of candidate bounds.
+ */
+private predicate isRecursiveBinary(BinaryOperation binop) {
+  (
+    binop instanceof UnsignedMulExpr
+    or
+    binop instanceof AddExpr
+    or
+    binop instanceof SubExpr
+  ) and
+  isRecursiveExpr(binop.getLeftOperand()) and
+  isRecursiveExpr(binop.getRightOperand())
+}
+
 /**
  * We distinguish 3 kinds of RangeSsaDefinition:
  *
@@ -581,7 +614,16 @@ private float getTruncatedLowerBounds(Expr expr) {
       // overflow, so we replace invalid bounds with exprMinVal.
       exists(float newLB | newLB = normalizeFloatUp(getLowerBoundsImpl(expr)) |
         if exprMinVal(expr) <= newLB and newLB <= exprMaxVal(expr)
-        then result = newLB
+        then
+          // Apply widening where we might get a combinatorial explosion.
+          if isRecursiveBinary(expr)
+          then
+            result =
+              max(float widenLB |
+                widenLB = wideningLowerBounds(expr.getUnspecifiedType()) and
+                not widenLB > newLB
+              )
+          else result = newLB
         else result = exprMinVal(expr)
       )
       or
@@ -628,7 +670,16 @@ private float getTruncatedUpperBounds(Expr expr) {
       // `exprMaxVal`.
       exists(float newUB | newUB = normalizeFloatUp(getUpperBoundsImpl(expr)) |
         if exprMinVal(expr) <= newUB and newUB <= exprMaxVal(expr)
-        then result = newUB
+        then
+          // Apply widening where we might get a combinatorial explosion.
+          if isRecursiveBinary(expr)
+          then
+            result =
+              min(float widenUB |
+                widenUB = wideningUpperBounds(expr.getUnspecifiedType()) and
+                not widenUB < newUB
+              )
+          else result = newUB
         else result = exprMaxVal(expr)
       )
       or

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

@@ -582,6 +582,12 @@
 | test.c:635:9:635:10 | ss | -32768 |
 | test.c:638:7:638:8 | ss | -32768 |
 | test.c:639:9:639:10 | ss | -1 |
+| test.c:645:8:645:8 | s | -2147483648 |
+| test.c:645:15:645:15 | s | 0 |
+| test.c:645:23:645:23 | s | 0 |
+| test.c:646:18:646:18 | s | 0 |
+| test.c:646:22:646:22 | s | 0 |
+| test.c:647:9:647:14 | result | 0 |
 | test.cpp:10:7:10:7 | b | -2147483648 |
 | test.cpp:11:5:11:5 | x | -2147483648 |
 | test.cpp:13:10:13:10 | x | -2147483648 |

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

@@ -638,4 +638,12 @@ void two_bounds_from_one_test(short ss, unsigned short us) {
   if (ss + 1 < sizeof(int)) {
     out(ss); // -1 .. 2
   }
+}
+
+void widen_recursive_expr() {
+  int s;
+  for (s = 0; s < 10; s++) {
+    int result = s + s; // 0 .. 9 [BUG: upper bound is 15 due to widening]
+    out(result); // 0 .. 18 [BUG: upper bound is 127 due to double widening]
+  }
 }

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

@@ -582,6 +582,12 @@
 | test.c:635:9:635:10 | ss | 32767 |
 | test.c:638:7:638:8 | ss | 32767 |
 | 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:647:9:647:14 | result | 127 |
 | test.cpp:10:7:10:7 | b | 2147483647 |
 | test.cpp:11:5:11:5 | x | 2147483647 |
 | test.cpp:13:10:13:10 | x | 2147483647 |