diff --git a/cmd/cloner/cloner.go b/cmd/cloner/cloner.go index ab4a7b22f..576edd2b1 100644 --- a/cmd/cloner/cloner.go +++ b/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) { diff --git a/cmd/cloner/cloner_test.go b/cmd/cloner/cloner_test.go index c0a946480..834d1be7b 100644 --- a/cmd/cloner/cloner_test.go +++ b/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{ diff --git a/cmd/cloner/clonerex/clonerex.go b/cmd/cloner/clonerex/clonerex.go index d17dbefc5..41626d3ae 100644 --- a/cmd/cloner/clonerex/clonerex.go +++ b/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 diff --git a/cmd/cloner/clonerex/clonerex_clone.go b/cmd/cloner/clonerex/clonerex_clone.go index 7d94688a3..9e21ff9d0 100644 --- a/cmd/cloner/clonerex/clonerex_clone.go +++ b/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, 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 } diff --git a/cmd/viewer/tests/tests_clone.go b/cmd/viewer/tests/tests_clone.go index 08cae87e2..3b4182aa0 100644 --- a/cmd/viewer/tests/tests_clone.go +++ b/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 diff --git a/net/dns/dns_clone.go b/net/dns/dns_clone.go index 32765ceb4..760cd514d 100644 --- a/net/dns/dns_clone.go +++ b/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...) diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index 8b966b621..f22656306 100644 --- a/tailcfg/tailcfg_clone.go +++ b/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 {