Merge pull request #16497 from github/max-schaefer/comparison-with-wi…

…der-type

Java: Add tests for `comparison-with-wider-type`.

java/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql

@@ -16,43 +16,20 @@
 import java
 import semmle.code.java.arithmetic.Overflow
 
-int leftWidth(ComparisonExpr e) { result = e.getLeftOperand().getType().(NumType).getWidthRank() }
+int widthRank(Expr e) { result = e.getType().(NumType).getWidthRank() }
 
-int rightWidth(ComparisonExpr e) { result = e.getRightOperand().getType().(NumType).getWidthRank() }
-
-abstract class WideningComparison extends BinaryExpr instanceof ComparisonExpr {
-  abstract Expr getNarrower();
-
-  abstract Expr getWider();
-}
-
-class LTWideningComparison extends WideningComparison {
-  LTWideningComparison() {
-    (this instanceof LEExpr or this instanceof LTExpr) and
-    leftWidth(this) < rightWidth(this)
-  }
-
-  override Expr getNarrower() { result = this.getLeftOperand() }
-
-  override Expr getWider() { result = this.getRightOperand() }
-}
-
-class GTWideningComparison extends WideningComparison {
-  GTWideningComparison() {
-    (this instanceof GEExpr or this instanceof GTExpr) and
-    leftWidth(this) > rightWidth(this)
-  }
-
-  override Expr getNarrower() { result = this.getRightOperand() }
-
-  override Expr getWider() { result = this.getLeftOperand() }
+predicate wideningComparison(ComparisonExpr c, Expr lesserOperand, Expr greaterOperand) {
+  lesserOperand = c.getLesserOperand() and
+  greaterOperand = c.getGreaterOperand() and
+  widthRank(lesserOperand) < widthRank(greaterOperand)
 }
 
-from WideningComparison c, LoopStmt l
+from ComparisonExpr c, LoopStmt l, Expr lesserOperand, Expr greaterOperand
 where
+  wideningComparison(c, lesserOperand, greaterOperand) and
   not c.getAnOperand().isCompileTimeConstant() and
   l.getCondition().getAChildExpr*() = c
 select c,
-  "Comparison between $@ of type " + c.getNarrower().getType().getName() + " and $@ of wider type " +
-    c.getWider().getType().getName() + ".", c.getNarrower(), "expression", c.getWider(),
+  "Comparison between $@ of type " + lesserOperand.getType().getName() + " and $@ of wider type " +
+    greaterOperand.getType().getName() + ".", lesserOperand, "expression", greaterOperand,
   "expression"

java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.expected

@@ -0,0 +1,2 @@
+| ComparisonWithWiderType.java:4:25:4:29 | ... < ... | Comparison between $@ of type int and $@ of wider type long. | ComparisonWithWiderType.java:4:25:4:25 | i | expression | ComparisonWithWiderType.java:4:29:4:29 | l | expression |
+| ComparisonWithWiderType.java:16:26:16:30 | ... > ... | Comparison between $@ of type byte and $@ of wider type short. | ComparisonWithWiderType.java:16:30:16:30 | b | expression | ComparisonWithWiderType.java:16:26:16:26 | c | expression |

java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.java

@@ -0,0 +1,27 @@
+public class ComparisonWithWiderType {
+    public void testLt(long l) {
+        // BAD: loop variable is an int, but the upper bound is a long
+        for (int i = 0; i < l; i++) {
+            System.out.println(i);
+        }
+
+        // GOOD: loop variable is a long
+        for (long i = 0; i < l; i++) {
+            System.out.println(i);
+        }
+    }
+
+    public void testGt(short c) {
+        // BAD: loop variable is a byte, but the upper bound is a short
+        for (byte b = 0; c > b; b++) {
+            System.out.println(b);
+        }
+    }
+
+    public void testLe(int i) {
+        // GOOD: loop variable is a long, and the upper bound is an int
+        for (long l = 0; l <= i; l++) {
+            System.out.println(l);
+        }
+    }
+}

java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-190/ComparisonWithWiderType.ql