From 06fe00006ac90bcd442b5d20f8a079bd18fbf731 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Tue, 12 Nov 2019 08:54:47 +0000
Subject: [PATCH] Conservatively handle indirect updates through pointer-type
 receiver.

Method references `x.m` where the receiver of `m` is a pointer implicitly take the address of `x`, so they should be treated much the same as `&x` in terms of data flow. (Ideally we'd make this explicit in the data-flow graph itself, but that's for another PR.)
---
 ql/src/semmle/go/dataflow/SSA.qll             |  5 +++++
 .../CompareIdenticalValues.expected           |  1 +
 .../CompareIdenticalValues/tst.go             | 22 +++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/ql/src/semmle/go/dataflow/SSA.qll b/ql/src/semmle/go/dataflow/SSA.qll
index d58aec962c9..be04265ab90 100644
--- a/ql/src/semmle/go/dataflow/SSA.qll
+++ b/ql/src/semmle/go/dataflow/SSA.qll
@@ -24,6 +24,11 @@ class SsaSourceVariable extends LocalVariable {
     // variables that have their address taken
     exists(AddressExpr addr | addr.getOperand().stripParens() = getAUse())
     or
+    exists(DataFlow::MethodReadNode mrn |
+      mrn.getReceiver() = getARead() and
+      mrn.getMethod().getReceiverType() instanceof PointerType
+    )
+    or
     // variables where there is an unresolved reference with the same name in the same
     // scope or a nested scope, suggesting that name resolution information may be incomplete
     exists(FunctionScope scope, FuncDef inner |
diff --git a/ql/test/query-tests/RedundantCode/CompareIdenticalValues/CompareIdenticalValues.expected b/ql/test/query-tests/RedundantCode/CompareIdenticalValues/CompareIdenticalValues.expected
index 7bfe348c2fc..d116669ad77 100644
--- a/ql/test/query-tests/RedundantCode/CompareIdenticalValues/CompareIdenticalValues.expected
+++ b/ql/test/query-tests/RedundantCode/CompareIdenticalValues/CompareIdenticalValues.expected
@@ -1,3 +1,4 @@
 | CompareIdenticalValues.go:9:3:9:8 | ...<=... | This expression compares $@ to itself. | CompareIdenticalValues.go:9:3:9:3 | y | an expression |
 | tst.go:6:9:6:14 | ...==... | This expression compares $@ to itself. | tst.go:6:9:6:9 | x | an expression |
+| tst.go:60:9:60:14 | ...==... | This expression compares $@ to itself. | tst.go:60:9:60:9 | y | an expression |
 | vp.go:16:9:16:38 | ...!=... | This expression compares $@ to itself. | vp.go:16:9:16:21 | call to GetLength | an expression |
diff --git a/ql/test/query-tests/RedundantCode/CompareIdenticalValues/tst.go b/ql/test/query-tests/RedundantCode/CompareIdenticalValues/tst.go
index db44c5df0c0..935e71bab99 100644
--- a/ql/test/query-tests/RedundantCode/CompareIdenticalValues/tst.go
+++ b/ql/test/query-tests/RedundantCode/CompareIdenticalValues/tst.go
@@ -37,3 +37,25 @@ func baz() bool {
 	bump(&x)
 	return x == 0
 }
+
+type counter int
+
+func (x *counter) bump() {
+	*x++
+}
+
+func (x counter) bimp() {
+	x++
+}
+
+func baz2() bool {
+	var x counter
+	x.bump()
+	return x == 0 // OK
+}
+
+func baz3() bool {
+	var y counter
+	y.bimp()
+	return y == 0 // NOT OK
+}