Merge pull request #3072 from geoffw0/gezero2

C++: Improvement to cpp/unsigned-comparison-zero

Author: jbj

change-notes/1.24/analysis-cpp.md

@@ -25,6 +25,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
 | Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) |  | This query is no longer run on LGTM. |
+| Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. |
 
 ## Changes to libraries
 

cpp/ql/src/Likely Bugs/Arithmetic/UnsignedGEZero.qll

@@ -19,15 +19,19 @@ class ConstantZero extends Expr {
  * Holds if `candidate` is an expression such that if it's unsigned then we
  * want an alert at `ge`.
  */
-private predicate lookForUnsignedAt(GEExpr ge, Expr candidate) {
-  // Base case: `candidate >= 0`
-  ge.getRightOperand() instanceof ConstantZero and
-  candidate = ge.getLeftOperand().getFullyConverted() and
-  // left operand was a signed or unsigned IntegralType before conversions
+private predicate lookForUnsignedAt(RelationalOperation ge, Expr candidate) {
+  // Base case: `candidate >= 0` (or `0 <= candidate`)
+  (
+    ge instanceof GEExpr or
+    ge instanceof LEExpr
+  ) and
+  ge.getLesserOperand() instanceof ConstantZero and
+  candidate = ge.getGreaterOperand().getFullyConverted() and
+  // left/greater operand was a signed or unsigned IntegralType before conversions
   // (not a pointer, checking a pointer >= 0 is an entirely different mistake)
   // (not an enum, as the fully converted type of an enum is compiler dependent
   //  so checking an enum >= 0 is always reasonable)
-  ge.getLeftOperand().getUnderlyingType() instanceof IntegralType
+  ge.getGreaterOperand().getUnderlyingType() instanceof IntegralType
   or
   // Recursive case: `...(largerType)candidate >= 0`
   exists(Conversion conversion |
@@ -37,7 +41,7 @@ private predicate lookForUnsignedAt(GEExpr ge, Expr candidate) {
   )
 }
 
-class UnsignedGEZero extends GEExpr {
+class UnsignedGEZero extends ComparisonOperation {
   UnsignedGEZero() {
     exists(Expr ue |
       lookForUnsignedAt(this, ue) and

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/UnsignedGEZero/UnsignedGEZero.c

@@ -112,4 +112,29 @@ void myFunction() {
 	assert(CHECK_RANGE(ui, 0, 10)); // reasonable use
 	assert(UI >= ZERO); // violation (not detected)
 	assert(ui GE 0); // violation
+
+	if ((unsigned char)si >= 0) { // violation
+	}
+	if ((unsigned char)(signed int)si >= 0) { // violation
+	}
+	if ((signed int)(unsigned char)si >= 0) { // violation
+	}
+	if ((unsigned char)(signed char)si >= 0) { // violation
+	}
+	if ((signed char)(unsigned char)si >= 0) {
+	}
+
+	if ((signed int)(unsigned char)(signed int)si >= 0) { // violation
+	}
+	if ((signed char)(unsigned char)(signed int)si >= 0) {
+	}
+	if ((signed int)(unsigned char)(signed char)si >= 0) { // violation
+	}
+
+	if (ui <= 0) {
+	}
+	if (0 <= ui) { // violation
+	}
+	if (0 < ui) {
+	}
 }

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/UnsignedGEZero/UnsignedGEZero.cpp

@@ -112,4 +112,29 @@ void myFunction() {
 	assert(CHECK_RANGE(ui, 0, 10)); // reasonable use
 	assert(UI >= ZERO); // violation (not detected)
 	assert(ui GE 0); // violation
+
+	if ((unsigned char)si >= 0) { // violation
+	}
+	if ((unsigned char)(signed int)si >= 0) { // violation
+	}
+	if ((signed int)(unsigned char)si >= 0) { // violation
+	}
+	if ((unsigned char)(signed char)si >= 0) { // violation
+	}
+	if ((signed char)(unsigned char)si >= 0) {
+	}
+
+	if ((signed int)(unsigned char)(signed int)si >= 0) { // violation
+	}
+	if ((signed char)(unsigned char)(signed int)si >= 0) {
+	}
+	if ((signed int)(unsigned char)(signed char)si >= 0) { // violation
+	}
+
+	if (ui <= 0) {
+	}
+	if (0 <= ui) { // violation
+	}
+	if (0 < ui) {
+	}
 }

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/UnsignedGEZero/UnsignedGEZero.expected

@@ -14,6 +14,13 @@
 | UnsignedGEZero.c:101:9:101:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.c:111:9:111:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.c:114:9:114:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:116:6:116:27 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:118:6:118:39 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:120:6:120:39 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:122:6:122:40 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:127:6:127:51 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:131:6:131:52 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.c:136:6:136:12 | ... <= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.cpp:40:6:40:12 | ... >= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.cpp:48:6:48:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.cpp:54:6:54:12 | ... >= ... | Pointless comparison of unsigned value to zero. |
@@ -29,3 +36,10 @@
 | UnsignedGEZero.cpp:101:9:101:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.cpp:111:9:111:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
 | UnsignedGEZero.cpp:114:9:114:15 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:116:6:116:27 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:118:6:118:39 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:120:6:120:39 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:122:6:122:40 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:127:6:127:51 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:131:6:131:52 | ... >= ... | Pointless comparison of unsigned value to zero. |
+| UnsignedGEZero.cpp:136:6:136:12 | ... <= ... | Pointless comparison of unsigned value to zero. |