diff --git a/change-notes/2020-08-07-negative-length-check.md b/change-notes/2020-08-07-negative-length-check.md new file mode 100644 index 00000000000..38db6f38d54 --- /dev/null +++ b/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. diff --git a/ql/src/RedundantCode/NegativeLengthCheck.qhelp b/ql/src/RedundantCode/NegativeLengthCheck.qhelp index cff802dae03..bec529f2c8e 100644 --- a/ql/src/RedundantCode/NegativeLengthCheck.qhelp +++ b/ql/src/RedundantCode/NegativeLengthCheck.qhelp @@ -10,7 +10,7 @@ never less than zero. Hence, checking whether the result of a call to len< is either redundant or indicates a logic mistake.

-The same applies to the built-in function cap. +The same applies to the built-in function cap, and to unsigned integer values.

diff --git a/ql/src/RedundantCode/NegativeLengthCheck.ql b/ql/src/RedundantCode/NegativeLengthCheck.ql index 2c55baddc9b..f1ae5409c2d 100644 --- a/ql/src/RedundantCode/NegativeLengthCheck.ql +++ b/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 + "." diff --git a/ql/test/query-tests/RedundantCode/NegativeLengthCheck/NegativeLengthCheck.expected b/ql/test/query-tests/RedundantCode/NegativeLengthCheck/NegativeLengthCheck.expected index 72fd7ee8cfd..33a060bb930 100644 --- a/ql/test/query-tests/RedundantCode/NegativeLengthCheck/NegativeLengthCheck.expected +++ b/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. | diff --git a/ql/test/query-tests/RedundantCode/NegativeLengthCheck/main.go b/ql/test/query-tests/RedundantCode/NegativeLengthCheck/main.go index 40c940de8f1..e97bf93b208 100644 --- a/ql/test/query-tests/RedundantCode/NegativeLengthCheck/main.go +++ b/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 +}