Merge pull request github#5780 from MathiasVP/cleanup-missingGuard-predicates-after-range-analysis-fix

C++: Cleanup missingGuardAgainstOverflow

Author: geoffw0

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

@@ -1626,7 +1626,9 @@ private module SimpleRangeAnalysisCached {
   private predicate exprThatCanOverflow(Expr e) {
     e instanceof UnaryArithmeticOperation or
     e instanceof BinaryArithmeticOperation or
-    e instanceof LShiftExpr
+    e instanceof AssignArithmeticOperation or
+    e instanceof LShiftExpr or
+    e instanceof AssignLShiftExpr
   }
 
   /**

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

@@ -99,12 +99,11 @@ VariableAccess varUse(LocalScopeVariable v) { result = v.getAnAccess() }
  * Holds if `e` potentially overflows and `use` is an operand of `e` that is not guarded.
  */
 predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
-  (
-    convertedExprMightOverflowPositively(e)
-    or
-    // Ensure that the predicate holds when range analysis cannot determine an upper bound
-    upperBound(e.getFullyConverted()) = exprMaxVal(e.getFullyConverted())
-  ) and
+  // Since `e` is guarenteed to be a `BinaryArithmeticOperation`, a `UnaryArithmeticOperation` or
+  // an `AssignArithmeticOperation` by the other constraints in this predicate, we know that
+  // `convertedExprMightOverflowPositively` will have a result even when `e` is not analyzable
+  // by `SimpleRangeAnalysis`.
+  convertedExprMightOverflowPositively(e) and
   use = e.getAnOperand() and
   exists(LocalScopeVariable v | use.getTarget() = v |
     // overflow possible if large
@@ -126,12 +125,11 @@ predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
  * Holds if `e` potentially underflows and `use` is an operand of `e` that is not guarded.
  */
 predicate missingGuardAgainstUnderflow(Operation e, VariableAccess use) {
-  (
-    convertedExprMightOverflowNegatively(e)
-    or
-    // Ensure that the predicate holds when range analysis cannot determine a lower bound
-    lowerBound(e.getFullyConverted()) = exprMinVal(e.getFullyConverted())
-  ) and
+  // Since `e` is guarenteed to be a `BinaryArithmeticOperation`, a `UnaryArithmeticOperation` or
+  // an `AssignArithmeticOperation` by the other constraints in this predicate, we know that
+  // `convertedExprMightOverflowNegatively` will have a result even when `e` is not analyzable
+  // by `SimpleRangeAnalysis`.
+  convertedExprMightOverflowNegatively(e) and
   use = e.getAnOperand() and
   exists(LocalScopeVariable v | use.getTarget() = v |
     // underflow possible if use is left operand and small

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

@@ -3,5 +3,4 @@
 | test.c:50:3:50:5 | sc3 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:49:9:49:16 | 127 | Extreme value |
 | test.c:59:3:59:5 | sc6 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:58:9:58:16 | 127 | Extreme value |
 | test.c:63:3:63:5 | sc8 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:62:9:62:16 | - ... | Extreme value |
-| test.c:75:3:75:5 | sc1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:74:9:74:16 | 127 | Extreme value |
 | test.c:124:9:124:9 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:118:17:118:23 | 2147483647 | Extreme value |

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

@@ -72,7 +72,7 @@ void test_negatives() {
   signed char sc1, sc2, sc3, sc4, sc5, sc6, sc7, sc8;
 
   sc1 = CHAR_MAX;
-  sc1 += 0; // GOOD [FALSE POSITIVE]
+  sc1 += 0; // GOOD
   sc1 += -1; // GOOD
   sc2 = CHAR_MIN;
   sc2 += -1; // BAD [NOT DETECTED]