Add new query ConstantLengthComparison.

change-notes/1.24/analysis-go.md

@@ -0,0 +1,12 @@
+# Improvements to Go analysis
+
+## New queries
+
+| **Query**                                                                 | **Tags**                                                                   | **Purpose**                                                                                                                                            |
+|---------------------------------------------------------------------------|----------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Constant length comparison (`go/constant-length-comparison`)     | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. |
+
+## Changes to existing queries
+
+| **Query**                                           | **Expected impact**          | **Change**                                                |
+|-----------------------------------------------------|------------------------------|-----------------------------------------------------------|

ql/src/InconsistentCode/ConstantLengthComparison.go

@@ -0,0 +1,10 @@
+package main
+
+func isPrefixOf(xs, ys []int) bool {
+	for i := 0; i < len(xs); i++ {
+		if len(ys) == 0 || xs[i] != ys[i] {
+			return false
+		}
+	}
+	return true
+}

ql/src/InconsistentCode/ConstantLengthComparison.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Indexing operations on arrays, slices or strings should use an index at most one less than the
+length. If the operation uses a variable index but checks the length against a constant, this may
+indicate a logic error which could lead to an out-of-bounds access.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Inspect the code closely to determine whether the length should be compared to the index variable
+instead. For loops that iterate over every element, using a <code>range</code> loop is better than
+explicit index manipulation.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows a method which checks whether slice <code>xs</code> is a prefix of slice
+<code>ys</code>:
+</p>
+<sample src="ConstantLengthComparison.go" />
+<p>
+A loop using an index variable <code>i</code> is used to iterate over the elements of
+<code>xs</code> and compare them to the corresponding elements of <code>ys</code>. However, the
+check to ensure that <code>i</code> is a valid index into <code>ys</code> is incorrectly specified
+as <code>len(ys) == 0</code>. Instead, the check should ensure that <code>len(ys)</code> is greater
+than <code>i</code>:
+</p>
+<sample src="ConstantLengthComparisonGood.go" />
+</example>
+
+<references>
+<li>The Go Programming Language Specification: <a href="https://golang.org/ref/spec#For_statements">For statements</a>.</li>
+<li>The Go Programming Language Specification: <a href="https://golang.org/ref/spec#Index_expressions">Index expressions</a>.</li>
+</references>
+</qhelp>

ql/src/InconsistentCode/ConstantLengthComparison.ql

@@ -0,0 +1,34 @@
+/**
+ * @name Constant length comparison
+ * @description Comparing the length of an array to a constant before indexing it using a
+ *              loop variable may indicate a logic error.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/constant-length-comparison
+ * @tags correctness
+ * @precision high
+ */
+
+import go
+
+from
+  ForStmt fs, Variable i, DataFlow::ElementReadNode idx, GVN a,
+  ControlFlow::ConditionGuardNode cond, DataFlow::CallNode lenA
+where
+  // `i` is incremented in `fs`
+  fs.getPost().(IncStmt).getExpr() = i.getAReference() and
+  // `idx` reads `a[i]`
+  idx.reads(a.getANode(), i.getARead()) and
+  // `lenA` is `len(a)`
+  lenA = Builtin::len().getACall() and
+  lenA.getArgument(0) = a.getANode() and
+  // and is checked against a constant
+  exists(DataFlow::Node const | exists(const.getIntValue()) |
+    cond.ensuresNeq(lenA, const) or
+    cond.ensuresLeq(const, lenA, _)
+  ) and
+  cond.dominates(idx.asInstruction().getBasicBlock()) and
+  // and that check happens inside the loop body
+  cond.getCondition().getParent+() = fs.getBody()
+select cond.getCondition(),
+  "This checks the length against a constant, but it is indexed using a variable $@.", idx, "here"

ql/src/InconsistentCode/ConstantLengthComparisonGood.go

@@ -0,0 +1,10 @@
+package main
+
+func isPrefixOfGood(xs, ys []int) bool {
+	for i := 0; i < len(xs); i++ {
+		if len(ys) <= i || xs[i] != ys[i] {
+			return false
+		}
+	}
+	return true
+}

ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.expected

@@ -0,0 +1 @@
+| ConstantLengthComparison.go:5:6:5:17 | ...==... | This checks the length against a constant, but it is indexed using a variable $@. | ConstantLengthComparison.go:5:31:5:35 | index expression | here |

ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.go

@@ -0,0 +1,10 @@
+package main
+
+func isPrefixOf(xs, ys []int) bool {
+	for i := 0; i < len(xs); i++ {
+		if len(ys) == 0 || xs[i] != ys[i] { // NOT OK
+			return false
+		}
+	}
+	return true
+}

ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.qlref

@@ -0,0 +1 @@
+InconsistentCode/ConstantLengthComparison.ql

ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparisonGood.go

@@ -0,0 +1,10 @@
+package main
+
+func isPrefixOfGood(xs, ys []int) bool {
+	for i := 0; i < len(xs); i++ {
+		if len(ys) <= i || xs[i] != ys[i] { // OK
+			return false
+		}
+	}
+	return true
+}

ql/test/query-tests/InconsistentCode/ConstantLengthComparison/tst.go

@@ -0,0 +1,14 @@
+package main
+
+func isPrefixOfGood2(xs, ys []int) bool {
+	if len(ys) == 0 {  // OK: not inside the loop
+		return len(xs) == 0
+	}
+
+	for i := 0; i < len(xs); i++ {
+		if len(ys) <= i || xs[i] != ys[i] {
+			return false
+		}
+	}
+	return true
+}