cmd/cloner: deep-clone pointer elements in map-of-slice values

The cloner's codegen for map[K][]*V fields was doing a shallow
append (copying pointer values) instead of cloning each element.
This meant that cloned structs aliased the original's pointed-to
values through the map's slice entries.

Mirror the existing standalone-slice logic that checks
ContainsPointers(sliceType.Elem()) and generates per-element
cloning for pointer, interface, and struct types.

Regenerate net/dns and tailcfg which both had affected
map[...][]*dnstype.Resolver fields.

Fixes #19284

Signed-off-by: Andrew Dunham <andrew@tailscale.com>

cmd/cloner/cloner.go

@@ -143,25 +143,9 @@ func gen(buf *bytes.Buffer, it *codegen.ImportTracker, typ *types.Named) {
 				writef("if src.%s != nil {", fname)
 				writef("dst.%s = make([]%s, len(src.%s))", fname, n, fname)
 				writef("for i := range dst.%s {", fname)
-				if ptr, isPtr := ft.Elem().(*types.Pointer); isPtr {
-					writef("if src.%s[i] == nil { dst.%s[i] = nil } else {", fname, fname)
-					if codegen.ContainsPointers(ptr.Elem()) {
-						if _, isIface := ptr.Elem().Underlying().(*types.Interface); isIface {
-							writef("\tdst.%s[i] = new((*src.%s[i]).Clone())", fname, fname)
-						} else {
-							writef("\tdst.%s[i] = src.%s[i].Clone()", fname, fname)
-						}
-					} else {
-						writef("\tdst.%s[i] = new(*src.%s[i])", fname, fname)
-					}
-					writef("}")
-				} else if ft.Elem().String() == "encoding/json.RawMessage" {
-					writef("\tdst.%s[i] = append(src.%s[i][:0:0], src.%s[i]...)", fname, fname, fname)
-				} else if _, isIface := ft.Elem().Underlying().(*types.Interface); isIface {
-					writef("\tdst.%s[i] = src.%s[i].Clone()", fname, fname)
-				} else {
-					writef("\tdst.%s[i] = *src.%s[i].Clone()", fname, fname)
-				}
+				writeSliceElemClone(writef, ft.Elem(),
+					fmt.Sprintf("src.%s[i]", fname),
+					fmt.Sprintf("dst.%s[i]", fname))
 				writef("}")
 				writef("}")
 			} else {
@@ -189,11 +173,27 @@ func gen(buf *bytes.Buffer, it *codegen.ImportTracker, typ *types.Named) {
 				n := it.QualifiedName(sliceType.Elem())
 				writef("if dst.%s != nil {", fname)
 				writef("\tdst.%s = map[%s]%s{}", fname, it.QualifiedName(ft.Key()), it.QualifiedName(elem))
-				writef("\tfor k := range src.%s {", fname)
-				// use zero-length slice instead of nil to ensure
-				// the key is always copied.
-				writef("\t\tdst.%s[k] = append([]%s{}, src.%s[k]...)", fname, n, fname)
-				writef("\t}")
+				if codegen.ContainsPointers(sliceType.Elem()) {
+					writef("\tfor k, sv := range src.%s {", fname)
+					writef("\t\tif sv == nil {")
+					writef("\t\t\tcontinue")
+					writef("\t\t}")
+					writef("\t\tdst.%s[k] = make([]%s, len(sv))", fname, n)
+					writef("\t\tfor i := range sv {")
+					innerWritef := func(format string, args ...any) {
+						writef("\t\t"+format, args...)
+					}
+					writeSliceElemClone(innerWritef, sliceType.Elem(),
+						"sv[i]", fmt.Sprintf("dst.%s[k][i]", fname))
+					writef("\t\t}")
+					writef("\t}")
+				} else {
+					writef("\tfor k := range src.%s {", fname)
+					// use zero-length slice instead of nil to ensure
+					// the key is always copied.
+					writef("\t\tdst.%s[k] = append([]%s{}, src.%s[k]...)", fname, n, fname)
+					writef("\t}")
+				}
 				writef("}")
 			} else if codegen.IsViewType(elem) || !codegen.ContainsPointers(elem) {
 				// If the map values are view types (which are
@@ -242,6 +242,31 @@ func gen(buf *bytes.Buffer, it *codegen.ImportTracker, typ *types.Named) {
 	buf.Write(codegen.AssertStructUnchanged(t, name, typeParams, "Clone", it))
 }
 
+// writeSliceElemClone generates code to deep-clone a single slice element
+// from srcExpr to dstExpr. It handles pointer, json.RawMessage, interface,
+// and named struct element types.
+func writeSliceElemClone(writef func(string, ...any), elemType types.Type, srcExpr, dstExpr string) {
+	if ptr, isPtr := elemType.(*types.Pointer); isPtr {
+		writef("if %s == nil { %s = nil } else {", srcExpr, dstExpr)
+		if codegen.ContainsPointers(ptr.Elem()) {
+			if _, isIface := ptr.Elem().Underlying().(*types.Interface); isIface {
+				writef("\t%s = new((*%s).Clone())", dstExpr, srcExpr)
+			} else {
+				writef("\t%s = %s.Clone()", dstExpr, srcExpr)
+			}
+		} else {
+			writef("\t%s = new(*%s)", dstExpr, srcExpr)
+		}
+		writef("}")
+	} else if elemType.String() == "encoding/json.RawMessage" {
+		writef("%s = append(%s[:0:0], %s...)", dstExpr, srcExpr, srcExpr)
+	} else if _, isIface := elemType.Underlying().(*types.Interface); isIface {
+		writef("%s = %s.Clone()", dstExpr, srcExpr)
+	} else {
+		writef("%s = *%s.Clone()", dstExpr, srcExpr)
+	}
+}
+
 // hasBasicUnderlying reports true when typ.Underlying() is a slice or a map.
 func hasBasicUnderlying(typ types.Type) bool {
 	switch typ.Underlying().(type) {

cmd/cloner/cloner_test.go

@@ -182,6 +182,32 @@ func TestNamedMapContainer(t *testing.T) {
 	}
 }
 
+func TestMapSlicePointerContainer(t *testing.T) {
+	num := 42
+	orig := &clonerex.MapSlicePointerContainer{
+		Routes: map[string][]*clonerex.SliceContainer{
+			"route1": {
+				{Slice: []*int{&num}},
+				{Slice: []*int{&num, &num}},
+			},
+			"route2": {
+				{Slice: []*int{&num}},
+			},
+		},
+	}
+
+	cloned := orig.Clone()
+	if !reflect.DeepEqual(orig, cloned) {
+		t.Errorf("Clone() = %v, want %v", cloned, orig)
+	}
+
+	// Mutate cloned.Routes pointer values
+	*cloned.Routes["route1"][0].Slice[0] = 999
+	if *orig.Routes["route1"][0].Slice[0] == 999 {
+		t.Errorf("Clone() aliased memory in Routes: original was modified")
+	}
+}
+
 func TestDeeplyNestedMap(t *testing.T) {
 	num := 123
 	orig := &clonerex.DeeplyNestedMap{

cmd/cloner/clonerex/clonerex.go

@@ -1,7 +1,7 @@
 // Copyright (c) Tailscale Inc & contributors
 // SPDX-License-Identifier: BSD-3-Clause
 
-//go:generate go run tailscale.com/cmd/cloner  -clonefunc=true -type SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap,NamedMapContainer
+//go:generate go run tailscale.com/cmd/cloner  -clonefunc=true -type SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap,NamedMapContainer,MapSlicePointerContainer
 
 // Package clonerex is an example package for the cloner tool.
 package clonerex
@@ -60,6 +60,13 @@ type NamedMapContainer struct {
 	Attrs NamedMap
 }
 
+// MapSlicePointerContainer has a map whose values are slices of pointers.
+// This tests that the cloner deep-clones the pointer elements in the slice,
+// not just the slice itself (which would leave aliased pointers).
+type MapSlicePointerContainer struct {
+	Routes map[string][]*SliceContainer
+}
+
 // DeeplyNestedMap tests arbitrary depth of map nesting (3+ levels)
 type DeeplyNestedMap struct {
 	ThreeLevels map[string]map[string]map[string]int

cmd/cloner/clonerex/clonerex_clone.go

@@ -176,9 +176,41 @@ var _NamedMapContainerCloneNeedsRegeneration = NamedMapContainer(struct {
 	Attrs NamedMap
 }{})
 
+// Clone makes a deep copy of MapSlicePointerContainer.
+// The result aliases no memory with the original.
+func (src *MapSlicePointerContainer) Clone() *MapSlicePointerContainer {
+	if src == nil {
+		return nil
+	}
+	dst := new(MapSlicePointerContainer)
+	*dst = *src
+	if dst.Routes != nil {
+		dst.Routes = map[string][]*SliceContainer{}
+		for k, sv := range src.Routes {
+			if sv == nil {
+				continue
+			}
+			dst.Routes[k] = make([]*SliceContainer, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.Routes[k][i] = nil
+				} else {
+					dst.Routes[k][i] = sv[i].Clone()
+				}
+			}
+		}
+	}
+	return dst
+}
+
+// A compilation failure here means this code must be regenerated, with the command at the top of this file.
+var _MapSlicePointerContainerCloneNeedsRegeneration = MapSlicePointerContainer(struct {
+	Routes map[string][]*SliceContainer
+}{})
+
 // Clone duplicates src into dst and reports whether it succeeded.
 // To succeed, <src, dst> must be of types <*T, *T> or <*T, **T>,
-// where T is one of SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap,NamedMapContainer.
+// where T is one of SliceContainer,InterfaceContainer,MapWithPointers,DeeplyNestedMap,NamedMapContainer,MapSlicePointerContainer.
 func Clone(dst, src any) bool {
 	switch src := src.(type) {
 	case *SliceContainer:
@@ -226,6 +258,15 @@ func Clone(dst, src any) bool {
 			*dst = src.Clone()
 			return true
 		}
+	case *MapSlicePointerContainer:
+		switch dst := dst.(type) {
+		case *MapSlicePointerContainer:
+			*dst = *src.Clone()
+			return true
+		case **MapSlicePointerContainer:
+			*dst = src.Clone()
+			return true
+		}
 	}
 	return false
 }

cmd/viewer/tests/tests_clone.go

@@ -96,14 +96,34 @@ func (src *Map) Clone() *Map {
 	dst.StructWithoutPtr = maps.Clone(src.StructWithoutPtr)
 	if dst.SlicesWithPtrs != nil {
 		dst.SlicesWithPtrs = map[string][]*StructWithPtrs{}
-		for k := range src.SlicesWithPtrs {
-			dst.SlicesWithPtrs[k] = append([]*StructWithPtrs{}, src.SlicesWithPtrs[k]...)
+		for k, sv := range src.SlicesWithPtrs {
+			if sv == nil {
+				continue
+			}
+			dst.SlicesWithPtrs[k] = make([]*StructWithPtrs, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.SlicesWithPtrs[k][i] = nil
+				} else {
+					dst.SlicesWithPtrs[k][i] = sv[i].Clone()
+				}
+			}
 		}
 	}
 	if dst.SlicesWithoutPtrs != nil {
 		dst.SlicesWithoutPtrs = map[string][]*StructWithoutPtrs{}
-		for k := range src.SlicesWithoutPtrs {
-			dst.SlicesWithoutPtrs[k] = append([]*StructWithoutPtrs{}, src.SlicesWithoutPtrs[k]...)
+		for k, sv := range src.SlicesWithoutPtrs {
+			if sv == nil {
+				continue
+			}
+			dst.SlicesWithoutPtrs[k] = make([]*StructWithoutPtrs, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.SlicesWithoutPtrs[k][i] = nil
+				} else {
+					dst.SlicesWithoutPtrs[k][i] = new(*sv[i])
+				}
+			}
 		}
 	}
 	dst.StructWithoutPtrKey = maps.Clone(src.StructWithoutPtrKey)
@@ -115,8 +135,18 @@ func (src *Map) Clone() *Map {
 	}
 	if dst.SliceIntPtr != nil {
 		dst.SliceIntPtr = map[string][]*int{}
-		for k := range src.SliceIntPtr {
-			dst.SliceIntPtr[k] = append([]*int{}, src.SliceIntPtr[k]...)
+		for k, sv := range src.SliceIntPtr {
+			if sv == nil {
+				continue
+			}
+			dst.SliceIntPtr[k] = make([]*int, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.SliceIntPtr[k][i] = nil
+				} else {
+					dst.SliceIntPtr[k][i] = new(*sv[i])
+				}
+			}
 		}
 	}
 	dst.PointerKey = maps.Clone(src.PointerKey)
@@ -399,8 +429,14 @@ func (src *GenericCloneableStruct[T, V]) Clone() *GenericCloneableStruct[T, V] {
 	}
 	if dst.SliceMap != nil {
 		dst.SliceMap = map[string][]T{}
-		for k := range src.SliceMap {
-			dst.SliceMap[k] = append([]T{}, src.SliceMap[k]...)
+		for k, sv := range src.SliceMap {
+			if sv == nil {
+				continue
+			}
+			dst.SliceMap[k] = make([]T, len(sv))
+			for i := range sv {
+				dst.SliceMap[k][i] = sv[i].Clone()
+			}
 		}
 	}
 	return dst
@@ -500,14 +536,34 @@ func (src *StructWithTypeAliasFields) Clone() *StructWithTypeAliasFields {
 	}
 	if dst.MapOfSlicesWithPtrs != nil {
 		dst.MapOfSlicesWithPtrs = map[string][]*StructWithPtrsAlias{}
-		for k := range src.MapOfSlicesWithPtrs {
-			dst.MapOfSlicesWithPtrs[k] = append([]*StructWithPtrsAlias{}, src.MapOfSlicesWithPtrs[k]...)
+		for k, sv := range src.MapOfSlicesWithPtrs {
+			if sv == nil {
+				continue
+			}
+			dst.MapOfSlicesWithPtrs[k] = make([]*StructWithPtrsAlias, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.MapOfSlicesWithPtrs[k][i] = nil
+				} else {
+					dst.MapOfSlicesWithPtrs[k][i] = sv[i].Clone()
+				}
+			}
 		}
 	}
 	if dst.MapOfSlicesWithoutPtrs != nil {
 		dst.MapOfSlicesWithoutPtrs = map[string][]*StructWithoutPtrsAlias{}
-		for k := range src.MapOfSlicesWithoutPtrs {
-			dst.MapOfSlicesWithoutPtrs[k] = append([]*StructWithoutPtrsAlias{}, src.MapOfSlicesWithoutPtrs[k]...)
+		for k, sv := range src.MapOfSlicesWithoutPtrs {
+			if sv == nil {
+				continue
+			}
+			dst.MapOfSlicesWithoutPtrs[k] = make([]*StructWithoutPtrsAlias, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.MapOfSlicesWithoutPtrs[k][i] = nil
+				} else {
+					dst.MapOfSlicesWithoutPtrs[k][i] = new(*sv[i])
+				}
+			}
 		}
 	}
 	return dst

net/dns/dns_clone.go

@@ -33,8 +33,18 @@ func (src *Config) Clone() *Config {
 	}
 	if dst.Routes != nil {
 		dst.Routes = map[dnsname.FQDN][]*dnstype.Resolver{}
-		for k := range src.Routes {
-			dst.Routes[k] = append([]*dnstype.Resolver{}, src.Routes[k]...)
+		for k, sv := range src.Routes {
+			if sv == nil {
+				continue
+			}
+			dst.Routes[k] = make([]*dnstype.Resolver, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.Routes[k][i] = nil
+				} else {
+					dst.Routes[k][i] = sv[i].Clone()
+				}
+			}
 		}
 	}
 	dst.SearchDomains = append(src.SearchDomains[:0:0], src.SearchDomains...)

tailcfg/tailcfg_clone.go

@@ -262,8 +262,18 @@ func (src *DNSConfig) Clone() *DNSConfig {
 	}
 	if dst.Routes != nil {
 		dst.Routes = map[string][]*dnstype.Resolver{}
-		for k := range src.Routes {
-			dst.Routes[k] = append([]*dnstype.Resolver{}, src.Routes[k]...)
+		for k, sv := range src.Routes {
+			if sv == nil {
+				continue
+			}
+			dst.Routes[k] = make([]*dnstype.Resolver, len(sv))
+			for i := range sv {
+				if sv[i] == nil {
+					dst.Routes[k][i] = nil
+				} else {
+					dst.Routes[k][i] = sv[i].Clone()
+				}
+			}
 		}
 	}
 	if src.FallbackResolvers != nil {