From bdcb1f233cfceb6d61fc45caf74a7fd5f60c2f55 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max-schaefer@github.com>
Date: Mon, 24 Aug 2020 08:43:11 +0100
Subject: [PATCH 1/2] Prevent misoptimisation in `StringOps`.

---
 ql/src/semmle/go/StringOps.qll                | 45 ++++++++++++++-----
 .../go/dataflow/internal/DataFlowUtil.qll     |  7 +++
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/ql/src/semmle/go/StringOps.qll b/ql/src/semmle/go/StringOps.qll
index dfc8ceb3c9f..69008e02221 100644
--- a/ql/src/semmle/go/StringOps.qll
+++ b/ql/src/semmle/go/StringOps.qll
@@ -78,15 +78,25 @@ module StringOps {
     }
 
     /**
-     * An expression of form `strings.Index(A, B) === 0`.
+     * Holds if `eq` is of the form `nd == 0` or `nd != 0`.
+     */
+    pragma[noinline]
+    private predicate comparesToZero(DataFlow::EqualityTestNode eq, DataFlow::Node nd) {
+      exists(DataFlow::Node zero |
+        eq.hasOperands(globalValueNumber(nd).getANode(), zero) and
+        zero.getIntValue() = 0
+      )
+    }
+
+    /**
+     * An expression of form `strings.Index(A, B) == 0`.
      */
     private class HasPrefix_IndexOfEquals extends Range, DataFlow::EqualityTestNode {
       DataFlow::CallNode indexOf;
 
       HasPrefix_IndexOfEquals() {
-        indexOf.getTarget().hasQualifiedName("strings", "Index") and
-        getAnOperand() = globalValueNumber(indexOf).getANode() and
-        getAnOperand().getIntValue() = 0
+        comparesToZero(this, indexOf) and
+        indexOf.getTarget().hasQualifiedName("strings", "Index")
       }
 
       override DataFlow::Node getBaseString() { result = indexOf.getArgument(0) }
@@ -97,19 +107,30 @@ module StringOps {
     }
 
     /**
-     * A comparison of form `x[0] === 'k'` for some rune literal `k`.
+     * Holds if `eq` is of the form `str[0] == rhs` or `str[0] != rhs`.
+     */
+    pragma[noinline]
+    private predicate comparesFirstCharacter(
+      DataFlow::EqualityTestNode eq, DataFlow::Node str, DataFlow::Node rhs
+    ) {
+      exists(DataFlow::ElementReadNode read |
+        eq.hasOperands(globalValueNumber(read).getANode(), rhs) and
+        str = read.getBase() and
+        str.getType().getUnderlyingType() instanceof StringType and
+        read.getIndex().getIntValue() = 0
+      )
+    }
+
+    /**
+     * A comparison of form `x[0] == 'k'` for some rune literal `k`.
      */
     private class HasPrefix_FirstCharacter extends Range, DataFlow::EqualityTestNode {
-      DataFlow::ElementReadNode read;
+      DataFlow::Node base;
       DataFlow::Node runeLiteral;
 
-      HasPrefix_FirstCharacter() {
-        read.getBase().getType().getUnderlyingType() instanceof StringType and
-        read.getIndex().getIntValue() = 0 and
-        eq(_, globalValueNumber(read).getANode(), runeLiteral)
-      }
+      HasPrefix_FirstCharacter() { comparesFirstCharacter(this, base, runeLiteral) }
 
-      override DataFlow::Node getBaseString() { result = read.getBase() }
+      override DataFlow::Node getBaseString() { result = base }
 
       override DataFlow::Node getSubstring() { result = runeLiteral }
 
diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
index ed6958c73d3..4d4479482de 100644
--- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -669,6 +669,13 @@ class BinaryOperationNode extends Node {
 
   /** Gets the operator of this operation. */
   string getOperator() { result = op }
+
+  /** Holds if `x` and `y` are the operands of this operation, in either order. */
+  predicate hasOperands(Node x, Node y) {
+    x = getAnOperand() and
+    y = getAnOperand() and
+    x != y
+  }
 }
 
 /**

From 4c82ad60647d85fe7399cc3add92cfe455021791 Mon Sep 17 00:00:00 2001
From: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
Date: Tue, 25 Aug 2020 07:36:14 +0100
Subject: [PATCH 2/2] Apply suggestions from code review

Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com>
---
 ql/src/semmle/go/StringOps.qll | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ql/src/semmle/go/StringOps.qll b/ql/src/semmle/go/StringOps.qll
index 69008e02221..f844d945072 100644
--- a/ql/src/semmle/go/StringOps.qll
+++ b/ql/src/semmle/go/StringOps.qll
@@ -67,7 +67,7 @@ module StringOps {
     }
 
     /**
-     * An expression of form `strings.HasPrefix(A, B)`.
+     * An expression of the form `strings.HasPrefix(A, B)`.
      */
     private class StringsHasPrefix extends Range, DataFlow::CallNode {
       StringsHasPrefix() { getTarget().hasQualifiedName("strings", "HasPrefix") }
@@ -89,7 +89,7 @@ module StringOps {
     }
 
     /**
-     * An expression of form `strings.Index(A, B) == 0`.
+     * An expression of the form `strings.Index(A, B) == 0`.
      */
     private class HasPrefix_IndexOfEquals extends Range, DataFlow::EqualityTestNode {
       DataFlow::CallNode indexOf;
@@ -122,7 +122,7 @@ module StringOps {
     }
 
     /**
-     * A comparison of form `x[0] == 'k'` for some rune literal `k`.
+     * A comparison of the form `x[0] == 'k'` for some rune literal `k`.
      */
     private class HasPrefix_FirstCharacter extends Range, DataFlow::EqualityTestNode {
       DataFlow::Node base;
@@ -138,7 +138,7 @@ module StringOps {
     }
 
     /**
-     * A comparison of form `x[:len(y)] === y`.
+     * A comparison of the form `x[:len(y)] == y`.
      */
     private class HasPrefix_Substring extends Range, DataFlow::EqualityTestNode {
       DataFlow::SliceNode slice;