util/deephash: fix map hashing when key & element have the same type

Regression from 09afb8e35b, in which the
same reflect.Value scratch value was being used as the map iterator
copy destination.

Also: make nil and empty maps hash differently, add test.

Fixes #4871

Co-authored-by: Josh Bleecher Snyder <josharian@gmail.com>
Change-Id: I67f42524bc81f694c1b7259d6682200125ea4a66
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

util/deephash/deephash.go

@@ -98,10 +98,14 @@ func (s Sum) String() string {
 }
 
 var (
-	once sync.Once
+	seedOnce sync.Once
 	seed     uint64
 )
 
+func initSeed() {
+	seed = uint64(time.Now().UnixNano())
+}
+
 func (h *hasher) sum() (s Sum) {
 	h.bw.Flush()
 	// Sum into scratch & copy out, as hash.Hash is an interface
@@ -121,9 +125,7 @@ func Hash(v any) (s Sum) {
 	h := hasherPool.Get().(*hasher)
 	defer hasherPool.Put(h)
 	h.reset()
-	once.Do(func() {
-		seed = uint64(time.Now().UnixNano())
-	})
+	seedOnce.Do(initSeed)
 	h.hashUint64(seed)
 	h.hashValue(reflect.ValueOf(v), false)
 	return h.sum()
@@ -426,7 +428,7 @@ func (h *hasher) hashValueWithType(v reflect.Value, ti *typeInfo, forceCycleChec
 
 type mapHasher struct {
 	h               hasher
-	val  valueCache      // re-usable values for map iteration
+	valKey, valElem valueCache      // re-usable values for map iteration
 	iter            reflect.MapIter // re-usable map iterator
 }
 
@@ -461,8 +463,12 @@ func (h *hasher) hashMap(v reflect.Value, ti *typeInfo, checkCycles bool) {
 	defer iter.Reset(reflect.Value{}) // avoid pinning v from mh.iter when we return
 
 	var sum Sum
-	k := mh.val.get(v.Type().Key())
-	e := mh.val.get(v.Type().Elem())
+	if v.IsNil() {
+		sum.sum[0] = 1 // something non-zero
+	}
+
+	k := mh.valKey.get(v.Type().Key())
+	e := mh.valElem.get(v.Type().Elem())
 	mh.h.visitStack = h.visitStack // always use the parent's visit stack to avoid cycles
 	for iter.Next() {
 		k.SetIterKey(iter)

util/deephash/deephash_test.go

@@ -11,9 +11,11 @@ import (
 	"crypto/sha256"
 	"fmt"
 	"math"
+	"math/rand"
 	"reflect"
 	"runtime"
 	"testing"
+	"testing/quick"
 	"time"
 	"unsafe"
 
@@ -144,6 +146,40 @@ func TestIssue4868(t *testing.T) {
 	}
 }
 
+func TestIssue4871(t *testing.T) {
+	m1 := map[string]string{"": "", "x": "foo"}
+	m2 := map[string]string{}
+	if h1, h2 := Hash(m1), Hash(m2); h1 == h2 {
+		t.Errorf("bogus: h1=%x, h2=%x", h1, h2)
+	}
+}
+
+func TestNilVsEmptymap(t *testing.T) {
+	m1 := map[string]string(nil)
+	m2 := map[string]string{}
+	if h1, h2 := Hash(m1), Hash(m2); h1 == h2 {
+		t.Errorf("bogus: h1=%x, h2=%x", h1, h2)
+	}
+}
+
+func TestMapFraming(t *testing.T) {
+	m1 := map[string]string{"foo": "", "fo": "o"}
+	m2 := map[string]string{}
+	if h1, h2 := Hash(m1), Hash(m2); h1 == h2 {
+		t.Errorf("bogus: h1=%x, h2=%x", h1, h2)
+	}
+}
+
+func TestQuick(t *testing.T) {
+	initSeed()
+	err := quick.Check(func(v, w map[string]string) bool {
+		return (Hash(v) == Hash(w)) == reflect.DeepEqual(v, w)
+	}, &quick.Config{MaxCount: 1000, Rand: rand.New(rand.NewSource(int64(seed)))})
+	if err != nil {
+		t.Fatalf("seed=%v, err=%v", seed, err)
+	}
+}
+
 func getVal() []any {
 	return []any{
 		&wgcfg.Config{