From 1ff032d11e00f2b902346aba22c9b7ccc8eae3a4 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 21 Nov 2019 20:45:43 +0000
Subject: [PATCH] Add new query ConstantLengthComparison.

---
 change-notes/1.24/analysis-go.md              | 12 ++++++
 .../ConstantLengthComparison.go               | 10 +++++
 .../ConstantLengthComparison.qhelp            | 42 +++++++++++++++++++
 .../ConstantLengthComparison.ql               | 34 +++++++++++++++
 .../ConstantLengthComparisonGood.go           | 10 +++++
 .../ConstantLengthComparison.expected         |  1 +
 .../ConstantLengthComparison.go               | 10 +++++
 .../ConstantLengthComparison.qlref            |  1 +
 .../ConstantLengthComparisonGood.go           | 10 +++++
 .../ConstantLengthComparison/tst.go           | 14 +++++++
 10 files changed, 144 insertions(+)
 create mode 100644 change-notes/1.24/analysis-go.md
 create mode 100644 ql/src/InconsistentCode/ConstantLengthComparison.go
 create mode 100644 ql/src/InconsistentCode/ConstantLengthComparison.qhelp
 create mode 100644 ql/src/InconsistentCode/ConstantLengthComparison.ql
 create mode 100644 ql/src/InconsistentCode/ConstantLengthComparisonGood.go
 create mode 100644 ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.expected
 create mode 100644 ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.go
 create mode 100644 ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.qlref
 create mode 100644 ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparisonGood.go
 create mode 100644 ql/test/query-tests/InconsistentCode/ConstantLengthComparison/tst.go

diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md
new file mode 100644
index 00000000000..c3027844b97
--- /dev/null
+++ b/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**                                                |
+|-----------------------------------------------------|------------------------------|-----------------------------------------------------------|
diff --git a/ql/src/InconsistentCode/ConstantLengthComparison.go b/ql/src/InconsistentCode/ConstantLengthComparison.go
new file mode 100644
index 00000000000..3b9c12f963b
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/ql/src/InconsistentCode/ConstantLengthComparison.qhelp b/ql/src/InconsistentCode/ConstantLengthComparison.qhelp
new file mode 100644
index 00000000000..64df4b7aaf4
--- /dev/null
+++ b/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>
diff --git a/ql/src/InconsistentCode/ConstantLengthComparison.ql b/ql/src/InconsistentCode/ConstantLengthComparison.ql
new file mode 100644
index 00000000000..7ede82f1fdc
--- /dev/null
+++ b/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"
diff --git a/ql/src/InconsistentCode/ConstantLengthComparisonGood.go b/ql/src/InconsistentCode/ConstantLengthComparisonGood.go
new file mode 100644
index 00000000000..d28c931cd7e
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.expected b/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.expected
new file mode 100644
index 00000000000..06dbbbf4a52
--- /dev/null
+++ b/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 |
diff --git a/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.go b/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.go
new file mode 100644
index 00000000000..b2f7b02ab30
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.qlref b/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.qlref
new file mode 100644
index 00000000000..315838df15f
--- /dev/null
+++ b/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparison.qlref
@@ -0,0 +1 @@
+InconsistentCode/ConstantLengthComparison.ql
diff --git a/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparisonGood.go b/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/ConstantLengthComparisonGood.go
new file mode 100644
index 00000000000..9342124ed63
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/tst.go b/ql/test/query-tests/InconsistentCode/ConstantLengthComparison/tst.go
new file mode 100644
index 00000000000..38849492728
--- /dev/null
+++ b/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
+}
\ No newline at end of file