From 23ad0284140dfa17cd717683f0d96562ae702cea Mon Sep 17 00:00:00 2001
From: Joe Tsai <joetsai@digital-static.net>
Date: Wed, 21 Jul 2021 10:26:04 -0700
Subject: [PATCH] util/deephash: include type as part of hash for interfaces
 (#2476)

A Go interface may hold any number of different concrete types.
Just because two underlying values hash to the same thing
does not mean the two values are identical if they have different
concrete types. As such, include the type in the hash.
---
 util/deephash/deephash.go      | 21 ++++++++++++++++++++-
 util/deephash/deephash_test.go | 28 ++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/util/deephash/deephash.go b/util/deephash/deephash.go
index a8d8a1411..2c206b97e 100644
--- a/util/deephash/deephash.go
+++ b/util/deephash/deephash.go
@@ -202,7 +202,15 @@ func (h *hasher) print(v reflect.Value) (acyclic bool) {
 		}
 		return acyclic
 	case reflect.Interface:
-		return h.print(v.Elem())
+		if v.IsNil() {
+			w.WriteByte(0) // indicates nil
+			return true
+		}
+		v = v.Elem()
+
+		w.WriteByte(1) // indicates visiting interface value
+		h.hashType(v.Type())
+		return h.print(v)
 	case reflect.Map:
 		// TODO(bradfitz): ideally we'd avoid these map
 		// operations to detect cycles if we knew from the map
@@ -349,3 +357,14 @@ func (h *hasher) hashMapFallback(v reflect.Value) (acyclic bool) {
 	w.WriteString("}\n")
 	return acyclic
 }
+
+// hashType hashes a reflect.Type.
+// The hash is only consistent within the lifetime of a program.
+func (h *hasher) hashType(t reflect.Type) {
+	// This approach relies on reflect.Type always being backed by a unique
+	// *reflect.rtype pointer. A safer approach is to use a global sync.Map
+	// that maps reflect.Type to some arbitrary and unique index.
+	// While safer, it requires global state with memory that can never be GC'd.
+	rtypeAddr := reflect.ValueOf(t).Pointer() // address of *reflect.rtype
+	h.uint(uint64(rtypeAddr))
+}
diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go
index 0a2b28a14..6576c3fa2 100644
--- a/util/deephash/deephash_test.go
+++ b/util/deephash/deephash_test.go
@@ -5,6 +5,7 @@
 package deephash
 
 import (
+	"archive/tar"
 	"bufio"
 	"bytes"
 	"fmt"
@@ -21,6 +22,33 @@ import (
 	"tailscale.com/wgengine/wgcfg"
 )
 
+func TestHash(t *testing.T) {
+	type tuple [2]interface{}
+	type iface struct{ X interface{} }
+	type MyBool bool
+	type MyHeader tar.Header
+	tests := []struct {
+		in     tuple
+		wantEq bool
+	}{
+		{in: tuple{iface{MyBool(true)}, iface{MyBool(true)}}, wantEq: true},
+		{in: tuple{iface{true}, iface{MyBool(true)}}, wantEq: false},
+		{in: tuple{iface{MyHeader{}}, iface{MyHeader{}}}, wantEq: true},
+		{in: tuple{iface{MyHeader{}}, iface{tar.Header{}}}, wantEq: false},
+		{in: tuple{iface{&MyHeader{}}, iface{&MyHeader{}}}, wantEq: true},
+		{in: tuple{iface{&MyHeader{}}, iface{&tar.Header{}}}, wantEq: false},
+		{in: tuple{iface{[]map[string]MyBool{}}, iface{[]map[string]MyBool{}}}, wantEq: true},
+		{in: tuple{iface{[]map[string]bool{}}, iface{[]map[string]MyBool{}}}, wantEq: false},
+	}
+
+	for _, tt := range tests {
+		gotEq := Hash(tt.in[0]) == Hash(tt.in[1])
+		if gotEq != tt.wantEq {
+			t.Errorf("(Hash(%v) == Hash(%v)) = %v, want %v", tt.in[0], tt.in[1], gotEq, tt.wantEq)
+		}
+	}
+}
+
 func TestDeepHash(t *testing.T) {
 	// v contains the types of values we care about for our current callers.
 	// Mostly we're just testing that we don't panic on handled types.