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

@@ -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))
+}

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.