Merge pull request #2085 from aschackmull/java/overflow-check-fp

Java: Add another overflow check pattern to UselessComparisonTest.

Author: yh-semmle

change-notes/1.23/analysis-java.md

@@ -11,6 +11,7 @@ The following changes in version 1.23 affect Java analysis in all applications.
 | Query built from user-controlled sources (`java/sql-injection`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. |
 | Query built from local-user-controlled sources (`java/sql-injection-local`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. |
 | Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. |
+| Useless comparison test (`java/constant-comparison`) | Fewer false positives | Additional overflow check patterns are now recognized and no longer reported. |
 
 ## Changes to QL libraries
 

java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql

@@ -134,37 +134,48 @@ Expr overFlowCand() {
   result.(LocalVariableDeclExpr).getInit() = overFlowCand()
 }
 
-/** Gets an expression that equals `v` plus a positive value. */
-Expr increaseOfVar(SsaVariable v) {
+predicate positiveOrNegative(Expr e) { positive(e) or negative(e) }
+
+/** Gets an expression that equals `v` plus a positive or negative value. */
+Expr increaseOrDecreaseOfVar(SsaVariable v) {
   exists(AssignAddExpr add |
     result = add and
-    positive(add.getDest()) and
+    positiveOrNegative(add.getDest()) and
     add.getRhs() = v.getAUse()
   )
   or
   exists(AddExpr add, Expr e |
     result = add and
     add.hasOperands(v.getAUse(), e) and
-    positive(e)
+    positiveOrNegative(e)
   )
   or
-  exists(SsaExplicitUpdate x | result = x.getAUse() and x.getDefiningExpr() = increaseOfVar(v))
+  exists(SubExpr sub |
+    result = sub and
+    sub.getLeftOperand() = v.getAUse() and
+    positiveOrNegative(sub.getRightOperand())
+  )
   or
-  result.(ParExpr).getExpr() = increaseOfVar(v)
+  exists(SsaExplicitUpdate x |
+    result = x.getAUse() and x.getDefiningExpr() = increaseOrDecreaseOfVar(v)
+  )
   or
-  result.(AssignExpr).getRhs() = increaseOfVar(v)
+  result.(ParExpr).getExpr() = increaseOrDecreaseOfVar(v)
   or
-  result.(LocalVariableDeclExpr).getInit() = increaseOfVar(v)
+  result.(AssignExpr).getRhs() = increaseOrDecreaseOfVar(v)
+  or
+  result.(LocalVariableDeclExpr).getInit() = increaseOrDecreaseOfVar(v)
 }
 
 predicate overFlowTest(ComparisonExpr comp) {
-  exists(SsaVariable v |
-    comp.getLesserOperand() = increaseOfVar(v) and
-    comp.getGreaterOperand() = v.getAUse()
-  )
-  or
-  comp.getLesserOperand() = overFlowCand() and
-  comp.getGreaterOperand().(IntegerLiteral).getIntValue() = 0
+  (
+    exists(SsaVariable v | comp.hasOperands(increaseOrDecreaseOfVar(v), v.getAUse()))
+    or
+    comp.getLesserOperand() = overFlowCand() and
+    comp.getGreaterOperand().(IntegerLiteral).getIntValue() = 0
+  ) and
+  // exclude loop conditions as they are unlikely to be overflow tests
+  not comp.getEnclosingStmt() instanceof LoopStmt
 }
 
 predicate concurrentModificationTest(BinaryExpr test) {

java/ql/test/query-tests/UselessComparisonTest/A.java

@@ -121,6 +121,26 @@ void overflowTests(int x, int y) {
     }
   }
 
+  static final long VAL = 100L;
+
+  long overflowAwareIncrease(long x) {
+    if (x + VAL > x) {
+      return x + VAL;
+    } else {
+      overflow();
+      return Long.MAX_VALUE;
+    }
+  }
+
+  long overflowAwareDecrease(long x) {
+    if (x - VAL < x) {
+      return x - VAL;
+    } else {
+      overflow();
+      return Long.MIN_VALUE;
+    }
+  }
+
   void overflow() { }
 
   void unreachableCode() {