From ee723d8a4f111e430dcf4f10c41753cc88c63350 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Mon, 25 Nov 2019 15:03:46 +0000
Subject: [PATCH] Fix DeadStoreOfField false positive.

We should look into properly desugaring embedded types in the IR, but for now this workaround should suffice.
---
 change-notes/1.24/analysis-go.md              |  1 +
 ql/src/RedundantCode/DeadStoreOfField.ql      | 34 ++++++++++++++++++-
 .../RedundantCode/DeadStoreOfField/main.go    | 18 ++++++++++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md
index 1e9b8c40bd0..4028af888ae 100644
--- a/change-notes/1.24/analysis-go.md
+++ b/change-notes/1.24/analysis-go.md
@@ -11,3 +11,4 @@
 | **Query**                                           | **Expected impact**          | **Change**                                                |
 |-----------------------------------------------------|------------------------------|-----------------------------------------------------------|
 | Reflected cross-site scripting (`go/reflected-xss`) | Fewer results | Untrusted input flowing into an HTTP header definition is no longer flagged, since this is often harmless. |
+| Useless assignment to field (`go/useless-assignment-to-field`) | Fewer false positives | The query now conservatively handles fields promoted through embedded pointer types. |
diff --git a/ql/src/RedundantCode/DeadStoreOfField.ql b/ql/src/RedundantCode/DeadStoreOfField.ql
index 064912d35c7..f07ed59c6c0 100644
--- a/ql/src/RedundantCode/DeadStoreOfField.ql
+++ b/ql/src/RedundantCode/DeadStoreOfField.ql
@@ -53,6 +53,36 @@ predicate escapes(DataFlow::Node nd) {
   escapes(nd.getASuccessor())
 }
 
+/**
+ * Gets an embedded type `T` of `t`, with `isPtr` indicating whether
+ * the embedded field has the pointer type `*T` or just type `T`.
+ */
+Type getEmbeddedType(Type t, boolean isPtr) {
+  exists(Type embedded |
+    t.getUnderlyingType().(StructType).hasOwnField(_, _, embedded, true) |
+    if embedded instanceof PointerType then (
+      result = embedded.(PointerType).getBaseType() and
+      isPtr = true
+    ) else (
+      result = embedded and
+      isPtr = false
+    )
+  )
+}
+
+/** Gets an embedded type of `t`. */
+Type getEmbeddedType(Type t) {
+  result = getEmbeddedType(t, _)
+}
+
+/**
+ * Gets a transitive embedded type of `t`, where at least one of the embeddings goes through a
+ * pointer type.
+ */
+Type getTypeEmbeddedViaPointer(Type t) {
+  result = getEmbeddedType*(getEmbeddedType(getEmbeddedType*(t), true))
+}
+
 from Write w, LocalVariable v, Field f
 where
   // `w` writes `f` on `v`
@@ -62,5 +92,7 @@ where
   // exclude pointer-typed `v`; there may be reads through an alias
   not v.getType().getUnderlyingType() instanceof PointerType and
   // exclude escaping `v`; there may be reads in other functions
-  not exists(Read r | r.reads(v) | escapes(r))
+  not exists(Read r | r.reads(v) | escapes(r)) and
+  // exclude fields promoted through an embedded pointer type
+  not f = getTypeEmbeddedViaPointer(v.getType()).getField(_)
 select w, "This assignment to " + f + " is useless since its value is never read."
diff --git a/ql/test/query-tests/RedundantCode/DeadStoreOfField/main.go b/ql/test/query-tests/RedundantCode/DeadStoreOfField/main.go
index 8ea70820bc9..7f6bf057f9b 100644
--- a/ql/test/query-tests/RedundantCode/DeadStoreOfField/main.go
+++ b/ql/test/query-tests/RedundantCode/DeadStoreOfField/main.go
@@ -70,3 +70,21 @@ func test5(w wrapper) int {
 	w.hash = -1
 	return w.hash
 }
+
+type S struct {
+	x int
+}
+
+type T struct {
+	*S
+}
+
+func (t T) reset() {
+	t.x = 0 // OK: field is promoted through pointer type
+}
+
+func test6() int {
+	t := T{&S{1}}
+	t.reset()
+	return t.x
+}