From c75fb084ccee805a5d65f42cb7e4660a3662f952 Mon Sep 17 00:00:00 2001 From: phillip-stephens Date: Thu, 8 Aug 2024 11:28:21 -0400 Subject: [PATCH 1/6] added concat utility and test --- src/internal/util/util.go | 17 +++++++++++++++++ src/internal/util/util_test.go | 16 ++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/internal/util/util.go b/src/internal/util/util.go index 0176877f..df12a9c0 100644 --- a/src/internal/util/util.go +++ b/src/internal/util/util.go @@ -147,3 +147,20 @@ func RemoveDuplicates[T comparable](slice []T) []T { } return result } + +// Concat concatenates multiple slices of the same type into a new single slice of that type. +// Avoids a gotcha in Go where since append modifies the underlying memory of the input slice, doing +// newSlice := append(slice1, slice2) can modify slice1. See https://go.dev/doc/effective_go#append +// A std. library concat was added in go 1.22, but this is for backwards compatibility. https://pkg.go.dev/slices#Concat +// This should be used anytime you're not trying to modify the input slices. +func Concat[T any](slices ...[]T) []T { + var totalLen int + for _, s := range slices { + totalLen += len(s) + } + result := make([]T, 0, totalLen) + for _, s := range slices { + result = append(result, s...) + } + return result +} diff --git a/src/internal/util/util_test.go b/src/internal/util/util_test.go index bb4a9af4..90643591 100644 --- a/src/internal/util/util_test.go +++ b/src/internal/util/util_test.go @@ -143,3 +143,19 @@ func TestRemoveDuplicates(t *testing.T) { }) } } + +func TestConcat(t *testing.T) { + inputSlice1 := make([]int, 0, 10) + for i := 0; i < 3; i++ { + inputSlice1 = append(inputSlice1, i) + } + newSlice1 := Concat(inputSlice1, []int{4}) + newSlice2 := Concat(inputSlice1, []int{5}) + newSlice3 := Concat(inputSlice1, []int{6}) + require.Len(t, inputSlice1, 3) + require.Equal(t, 10, cap(inputSlice1)) + require.Equal(t, []int{0, 1, 2}, inputSlice1) + require.Equal(t, []int{0, 1, 2, 4}, newSlice1) + require.Equal(t, []int{0, 1, 2, 5}, newSlice2) + require.Equal(t, []int{0, 1, 2, 6}, newSlice3) +} From ea5d83614924bbb9c317bf27358ef7f7dc8d2562 Mon Sep 17 00:00:00 2001 From: phillip-stephens Date: Thu, 8 Aug 2024 11:33:44 -0400 Subject: [PATCH 2/6] added testing showing naive way doesn't work --- src/internal/util/util_test.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/internal/util/util_test.go b/src/internal/util/util_test.go index 90643591..10fb93c7 100644 --- a/src/internal/util/util_test.go +++ b/src/internal/util/util_test.go @@ -149,9 +149,17 @@ func TestConcat(t *testing.T) { for i := 0; i < 3; i++ { inputSlice1 = append(inputSlice1, i) } - newSlice1 := Concat(inputSlice1, []int{4}) - newSlice2 := Concat(inputSlice1, []int{5}) - newSlice3 := Concat(inputSlice1, []int{6}) + // Test naive append + newSlice1 := append(inputSlice1, 4) + newSlice2 := append(inputSlice1, 5) + newSlice3 := append(inputSlice1, 6) + // Shows test is working, you'd think that this would be Equal but it isn't. append() is modifying the inputSlice1 + require.NotEqual(t, []int{0, 1, 2, 4}, newSlice1) + require.NotEqual(t, []int{0, 1, 2, 5}, newSlice2) + // Now try with new Concat + newSlice1 = Concat(inputSlice1, []int{4}) + newSlice2 = Concat(inputSlice1, []int{5}) + newSlice3 = Concat(inputSlice1, []int{6}) require.Len(t, inputSlice1, 3) require.Equal(t, 10, cap(inputSlice1)) require.Equal(t, []int{0, 1, 2}, inputSlice1) From aa2554a2ba7d0dbd83490cb13ddf51c8c34379ec Mon Sep 17 00:00:00 2001 From: phillip-stephens Date: Thu, 8 Aug 2024 11:33:57 -0400 Subject: [PATCH 3/6] use new concat to fix bad appends --- src/zdns/alookup.go | 4 +++- src/zdns/lookup.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/zdns/alookup.go b/src/zdns/alookup.go index 9c72ee6c..68900cf9 100644 --- a/src/zdns/alookup.go +++ b/src/zdns/alookup.go @@ -18,6 +18,8 @@ import ( "github.com/pkg/errors" "github.com/zmap/dns" + + "github.com/zmap/zdns/src/internal/util" ) // DoTargetedLookup performs a lookup of the given domain name against the given nameserver, looking up both IPv4 and IPv6 addresses @@ -57,7 +59,7 @@ func (r *Resolver) DoTargetedLookup(name, nameServer string, ipMode IPVersionMod } } - combinedTrace := append(ipv4Trace, ipv6Trace...) + combinedTrace := util.Concat(ipv4Trace, ipv6Trace) // In case we get no IPs and a non-NOERROR status from either // IPv4 or IPv6 lookup, we return that status. diff --git a/src/zdns/lookup.go b/src/zdns/lookup.go index 30cd9138..eb91fed2 100644 --- a/src/zdns/lookup.go +++ b/src/zdns/lookup.go @@ -292,7 +292,7 @@ func (r *Resolver) LookupAllNameservers(q *Question, nameServer string) (*Combin for _, nserver := range nsResults.Servers { // Use all the ipv4 and ipv6 addresses of each nameserver nameserver := nserver.Name - ips := append(nserver.IPv4Addresses, nserver.IPv6Addresses...) + ips := util.Concat(nserver.IPv4Addresses, nserver.IPv6Addresses) for _, ip := range ips { curServer = net.JoinHostPort(ip, "53") res, trace, status, _ := r.ExternalLookup(q, curServer) From 05e467def294e7853cc8d01a3b62cf021cac7950 Mon Sep 17 00:00:00 2001 From: phillip-stephens Date: Thu, 8 Aug 2024 11:43:33 -0400 Subject: [PATCH 4/6] just copied std. lib.'s implementation to avoid any issues in rolling your own --- src/internal/util/util.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/internal/util/util.go b/src/internal/util/util.go index df12a9c0..763330be 100644 --- a/src/internal/util/util.go +++ b/src/internal/util/util.go @@ -19,6 +19,7 @@ import ( "fmt" "net" "regexp" + slices2 "slices" "strconv" "strings" @@ -148,19 +149,23 @@ func RemoveDuplicates[T comparable](slice []T) []T { return result } -// Concat concatenates multiple slices of the same type into a new single slice of that type. +// Concat returns a new slice concatenating the passed in slices. +// // Avoids a gotcha in Go where since append modifies the underlying memory of the input slice, doing // newSlice := append(slice1, slice2) can modify slice1. See https://go.dev/doc/effective_go#append // A std. library concat was added in go 1.22, but this is for backwards compatibility. https://pkg.go.dev/slices#Concat -// This should be used anytime you're not trying to modify the input slices. -func Concat[T any](slices ...[]T) []T { - var totalLen int +// This is a direct copy of the std. library implementation's Concat implementation. +func Concat[S ~[]E, E any](slices ...S) S { + size := 0 for _, s := range slices { - totalLen += len(s) + size += len(s) + if size < 0 { + panic("len out of range") + } } - result := make([]T, 0, totalLen) + newslice := slices2.Grow[S](nil, size) for _, s := range slices { - result = append(result, s...) + newslice = append(newslice, s...) } - return result + return newslice } From b6dc18c9493ac7dacc69f54b070887392252f0c3 Mon Sep 17 00:00:00 2001 From: phillip-stephens Date: Thu, 8 Aug 2024 11:45:22 -0400 Subject: [PATCH 5/6] lint --- src/internal/util/util_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/src/internal/util/util_test.go b/src/internal/util/util_test.go index 10fb93c7..c8ad0c3f 100644 --- a/src/internal/util/util_test.go +++ b/src/internal/util/util_test.go @@ -156,6 +156,7 @@ func TestConcat(t *testing.T) { // Shows test is working, you'd think that this would be Equal but it isn't. append() is modifying the inputSlice1 require.NotEqual(t, []int{0, 1, 2, 4}, newSlice1) require.NotEqual(t, []int{0, 1, 2, 5}, newSlice2) + require.Equal(t, []int{0, 1, 2, 6}, newSlice3) // Now try with new Concat newSlice1 = Concat(inputSlice1, []int{4}) newSlice2 = Concat(inputSlice1, []int{5}) From 0fea0b2517bc01b3f7aa4302fbddfe8a81fa976f Mon Sep 17 00:00:00 2001 From: phillip-stephens Date: Thu, 8 Aug 2024 12:09:50 -0400 Subject: [PATCH 6/6] resolved backwards compatability issue --- src/internal/util/util.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/internal/util/util.go b/src/internal/util/util.go index 763330be..8c883ea7 100644 --- a/src/internal/util/util.go +++ b/src/internal/util/util.go @@ -19,7 +19,6 @@ import ( "fmt" "net" "regexp" - slices2 "slices" "strconv" "strings" @@ -154,7 +153,7 @@ func RemoveDuplicates[T comparable](slice []T) []T { // Avoids a gotcha in Go where since append modifies the underlying memory of the input slice, doing // newSlice := append(slice1, slice2) can modify slice1. See https://go.dev/doc/effective_go#append // A std. library concat was added in go 1.22, but this is for backwards compatibility. https://pkg.go.dev/slices#Concat -// This is a direct copy of the std. library implementation's Concat implementation. +// This is mostly similiar to the std. library concat, but with a few differences so it compiles on go 1.20. func Concat[S ~[]E, E any](slices ...S) S { size := 0 for _, s := range slices { @@ -163,9 +162,9 @@ func Concat[S ~[]E, E any](slices ...S) S { panic("len out of range") } } - newslice := slices2.Grow[S](nil, size) + newSlice := make([]E, 0, size) for _, s := range slices { - newslice = append(newslice, s...) + newSlice = append(newSlice, s...) } - return newslice + return newSlice }