Merge pull request #280 from owen-mc/negative-length-check-unsigned

Extend negativeLengthCheck query to unsigned integers

Author: owen-mc

change-notes/2020-08-07-negative-length-check.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Query "Redundant check for negative value" (`go/negative-length-check`) has been expanded to consider unsigned integers, along
+  with the return values of `len` and `cap` which it already handled. It has also been renamed to match its expanded role.

ql/src/RedundantCode/NegativeLengthCheck.qhelp

@@ -10,7 +10,7 @@ never less than zero. Hence, checking whether the result of a call to <code>len<
 is either redundant or indicates a logic mistake.
 </p>
 <p>
-The same applies to the built-in function <code>cap</code>.
+The same applies to the built-in function <code>cap</code>, and to unsigned integer values.
 </p>
 </overview>
 

ql/src/RedundantCode/NegativeLengthCheck.ql

@@ -1,7 +1,9 @@
 /**
- * @name Check for negative length
+ * @name Redundant check for negative value
  * @description Checking whether the result of 'len' or 'cap' is negative is pointless,
- *              since these functions always returns a non-negative number.
+ *              since these functions always returns a non-negative number. It is also
+ *              pointless checking if an unsigned integer is negative, as it can only
+ *              hold non-negative values.
  * @kind problem
  * @problem.severity warning
  * @precision very-high
@@ -11,12 +13,19 @@
 
 import go
 
-from ComparisonExpr cmp, BuiltinFunction len, int ub, string r
+from ComparisonExpr cmp, DataFlow::Node op, int ub, string d, string r
 where
-  (len = Builtin::len() or len = Builtin::cap()) and
+  (
+    exists(BuiltinFunction bf | bf = Builtin::len() or bf = Builtin::cap() |
+      op = bf.getACall() and d = "'" + bf.getName() + "'"
+    )
+    or
+    op.getType().getUnderlyingType() instanceof UnsignedIntegerType and
+    d = "This unsigned value"
+  ) and
   (
     exists(RelationalComparisonExpr rel | rel = cmp |
-      rel.getLesserOperand() = len.getACall().asExpr() and
+      rel.getLesserOperand() = op.asExpr() and
       rel.getGreaterOperand().getIntValue() = ub and
       (
         ub < 0
@@ -27,10 +36,10 @@ where
     )
     or
     exists(EqualityTestExpr eq | eq = cmp |
-      eq.getAnOperand() = len.getACall().asExpr() and
+      eq.getAnOperand() = op.asExpr() and
       eq.getAnOperand().getIntValue() = ub and
       ub < 0 and
       r = "equal"
     )
   )
-select cmp, "'" + len.getName() + "' is always non-negative, and hence cannot " + r + " " + ub + "."
+select cmp, d + " is always non-negative, and hence cannot " + r + " " + ub + "."

ql/test/query-tests/RedundantCode/NegativeLengthCheck/NegativeLengthCheck.expected

@@ -3,3 +3,5 @@
 | main.go:14:5:14:20 | ...<... | 'cap' is always non-negative, and hence cannot be less than 0. |
 | main.go:18:5:18:22 | ...<=... | 'len' is always non-negative, and hence cannot be less than -1. |
 | main.go:22:5:22:22 | ...==... | 'len' is always non-negative, and hence cannot equal -1. |
+| main.go:28:9:28:13 | ...<... | This unsigned value is always non-negative, and hence cannot be less than 0. |
+| main.go:36:9:36:15 | ...==... | This unsigned value is always non-negative, and hence cannot equal -1. |

ql/test/query-tests/RedundantCode/NegativeLengthCheck/main.go

@@ -23,3 +23,15 @@ func main() {
 		println("No arguments provided.")
 	}
 }
+
+func checkNegative(x uint) bool {
+	return x < 0 // NOT OK
+}
+
+func checkNonPositive(x uint) bool {
+	return x <= 0 // OK
+}
+
+func checkIsMinusOne(x uint) bool {
+	return x == -1 // NOT OK
+}