diff --git a/src/net/dnsclient.go b/src/net/dnsclient.go index eb509d175fc1a0..497d4a38d88ed9 100644 --- a/src/net/dnsclient.go +++ b/src/net/dnsclient.go @@ -27,6 +27,17 @@ func randIntn(n int) int { return randInt() % n } +// shuffle is a copy of math/rand/v2.Shuffle. +func shuffle(n int, swap func(i, j int)) { + if n < 0 { + panic("invalid argument to Shuffle") + } + for i := n - 1; i > 0; i-- { + j := randIntn(i + 1) + swap(i, j) + } +} + // reverseaddr returns the in-addr.arpa. or ip6.arpa. hostname of the IP // address addr suitable for rDNS (PTR) record lookup or an error if it fails // to parse the IP address. diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index 6e749882c25c6a..d48f9343500146 100644 --- a/src/net/dnsclient_unix.go +++ b/src/net/dnsclient_unix.go @@ -624,7 +624,6 @@ func goLookupIPFiles(name string) (addrs []IPAddr, canonical string) { addrs = append(addrs, addr) } } - sortByRFC6724(addrs) return addrs, canonical } @@ -636,6 +635,12 @@ func (r *Resolver) goLookupIP(ctx context.Context, network, host string, order h } func (r *Resolver) goLookupIPCNAMEOrder(ctx context.Context, network, name string, order hostLookupOrder, conf *dnsConfig) (addrs []IPAddr, cname dnsmessage.Name, err error) { + addrs, cname, err = r.goLookupIPCNAME(ctx, network, name, order, conf) + sortByRFC6724(addrs) + return addrs, cname, err +} + +func (r *Resolver) goLookupIPCNAME(ctx context.Context, network, name string, order hostLookupOrder, conf *dnsConfig) (addrs []IPAddr, cname dnsmessage.Name, err error) { if order == hostLookupFilesDNS || order == hostLookupFiles { var canonical string addrs, canonical = goLookupIPFiles(name) @@ -825,7 +830,6 @@ func (r *Resolver) goLookupIPCNAMEOrder(ctx context.Context, network, name strin // just one is misleading. See also golang.org/issue/6324. lastErr.Name = name } - sortByRFC6724(addrs) if len(addrs) == 0 && !(network == "CNAME" && cname.Length > 0) { if order == hostLookupDNSFiles { var canonical string diff --git a/src/net/hook.go b/src/net/hook.go index 08d1aa893481f2..9a8252bdccea40 100644 --- a/src/net/hook.go +++ b/src/net/hook.go @@ -28,4 +28,6 @@ var ( // short deadline (such as 1ns in the future) is always expired by the time // a relevant system call occurs. testHookStepTime = func() {} + + testHookShuffleRand = shuffle ) diff --git a/src/net/lookup.go b/src/net/lookup.go index 06e14dfdda796f..a3c250bf27ab97 100644 --- a/src/net/lookup.go +++ b/src/net/lookup.go @@ -373,18 +373,30 @@ func (r *Resolver) lookupIPAddr(ctx context.Context, network, host string) ([]IP addrs, _ := r.Val.([]IPAddr) trace.DNSDone(ipAddrsEface(addrs), r.Shared, err) } - return lookupIPReturn(r.Val, err, r.Shared) + // Shuffled before sorting per RFC 6724, so concurrent callers do not always receive the same order. + // See https://go.dev/issue/34511. + // See https://go.dev/issue/31698. + addrs, err := lookupIPReturn(r.Val, err, r.Shared) + testHookShuffleRand(len(addrs), func(i, j int) { + addrs[i], addrs[j] = addrs[j], addrs[i] + }) + sortByRFC6724(addrs) + return addrs, err } } // lookupIPReturn turns the return values from singleflight.Do into // the return values from LookupIP. +// +// The caller may shuffle and sort the result in place, +// and the underlying slice is shared among all concurrent singleflight callers, +// so it must not be mutated directly. func lookupIPReturn(addrsi any, err error, shared bool) ([]IPAddr, error) { if err != nil { return nil, err } addrs := addrsi.([]IPAddr) - if shared { + if len(addrs) > 1 && shared { clone := make([]IPAddr, len(addrs)) copy(clone, addrs) addrs = clone diff --git a/src/net/lookup_test.go b/src/net/lookup_test.go index afa7e4c14aaf1f..8964db447005c6 100644 --- a/src/net/lookup_test.go +++ b/src/net/lookup_test.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "internal/testenv" + "math/rand/v2" "net/netip" "reflect" "runtime" @@ -17,6 +18,7 @@ import ( "sync" "sync/atomic" "testing" + "testing/synctest" "time" ) @@ -1132,12 +1134,43 @@ func TestLookupIPAddrPreservesContextValues(t *testing.T) { if err != nil { t.Errorf("Resolver #%d: unexpected error: %v", i, err) } - if !reflect.DeepEqual(gotIPs, wantIPs) { + // Ignore order + if !reflect.DeepEqual(sortedIPAddrStrings(gotIPs), sortedIPAddrStrings(wantIPs)) { t.Errorf("#%d: mismatched IPAddr results\n\tGot: %v\n\tWant: %v", i, gotIPs, wantIPs) } } } +type lockedRand struct { + mu sync.Mutex + r *rand.Rand +} + +func newLockedRand(seed1, seed2 uint64) *lockedRand { + return &lockedRand{r: rand.New(rand.NewPCG(seed1, seed2))} +} + +func (lr *lockedRand) IntN(n int) int { + lr.mu.Lock() + defer lr.mu.Unlock() + return lr.r.IntN(n) +} + +func (lr *lockedRand) Shuffle(n int, swap func(i, j int)) { + lr.mu.Lock() + defer lr.mu.Unlock() + lr.r.Shuffle(n, swap) +} + +func sortedIPAddrStrings(ipAddrs []IPAddr) []string { + ret := make([]string, len(ipAddrs)) + for i, ipAddr := range ipAddrs { + ret[i] = ipAddr.String() + "\000" + ipAddr.Zone + } + slices.Sort(ret) + return ret +} + // Issue 30521: The lookup group should call the resolver for each network. func TestLookupIPAddrConcurrentCallsForNetworks(t *testing.T) { origTestHookLookupIP := testHookLookupIP @@ -1193,7 +1226,8 @@ func TestLookupIPAddrConcurrentCallsForNetworks(t *testing.T) { t.Errorf("lookupIPAddr(%v, %v): unexpected error: %v", network, host, err) } wantIPs := results[[2]string{network, host}] - if !reflect.DeepEqual(gotIPs, wantIPs) { + // Ignore order + if !reflect.DeepEqual(sortedIPAddrStrings(gotIPs), sortedIPAddrStrings(wantIPs)) { t.Errorf("lookupIPAddr(%v, %v): mismatched IPAddr results\n\tGot: %v\n\tWant: %v", network, host, gotIPs, wantIPs) } }() @@ -1201,6 +1235,63 @@ func TestLookupIPAddrConcurrentCallsForNetworks(t *testing.T) { wg.Wait() } +// Issue 31698: Concurrent callers do not always receive the same order. +func TestLookupIPAddrConcurrentCallsForShuffle(t *testing.T) { + synctest.Test(t, func(*testing.T) { + origTestHookLookupIP := testHookLookupIP + defer func() { testHookLookupIP = origTestHookLookupIP }() + + origTestHookShuffleRand := testHookShuffleRand + defer func() { testHookShuffleRand = origTestHookShuffleRand }() + + // Both addresses share the same commonPrefixLen, so the RFC 6724 sort calls + // Shuffle only once; with a fixed seed the pseudo-random result is predictable. + ipv4LocalHost := []IPAddr{ + {IP: IPv4(127, 0, 0, 2)}, + {IP: IPv4(127, 0, 0, 3)}, + } + testHookLookupIP = func(ctx context.Context, fn func(context.Context, string, string) ([]IPAddr, error), network, host string) ([]IPAddr, error) { + // Simulation for DNS query time cost + time.Sleep(50 * time.Millisecond) + return ipv4LocalHost, nil + } + // Use a locked random source so concurrent Shuffle calls do not race on shared state. + // With this deliberately chosen fixed seed the two concurrent lookups + // stay deterministic, so just two lookups suffice to verify each caller + // receives a shuffled result. + testHookShuffleRand = newLockedRand(0, 0).Shuffle + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + network := "udp" + host := "golang.org" + result := make([][]IPAddr, 0, 2) + var mu sync.Mutex + for range 2 { + go func() { + gotIPs, err := DefaultResolver.lookupIPAddr(ctx, network, host) + if err != nil { + t.Errorf("lookupIPAddr(%v, %v): unexpected error: %v", network, host, err) + } + // Ignore order + if !reflect.DeepEqual(sortedIPAddrStrings(gotIPs), sortedIPAddrStrings(ipv4LocalHost)) { + t.Errorf("lookupIPAddr(%v, %v): mismatched IPAddr results\n\tGot: %v\n\tWant: %v", network, host, gotIPs, ipv4LocalHost) + } + mu.Lock() + defer mu.Unlock() + result = append(result, gotIPs) + }() + } + synctest.Sleep(50 * time.Millisecond) + for i := 1; i < len(result); i++ { + // Lookups always return the same order (expected shuffled results) + if !reflect.DeepEqual(result[i], result[0]) { + return + } + } + t.Errorf("Lookups always return the same order (expected shuffled results)") + }) +} + // Issue 53995: Resolver.LookupIP should return error for empty host name. func TestResolverLookupIPWithEmptyHost(t *testing.T) { _, err := DefaultResolver.LookupIP(context.Background(), "ip", "") diff --git a/src/net/lookup_unix.go b/src/net/lookup_unix.go index 86108939cd03d6..d8fe2ef5c8e491 100644 --- a/src/net/lookup_unix.go +++ b/src/net/lookup_unix.go @@ -63,7 +63,10 @@ func (r *Resolver) lookupIP(ctx context.Context, network, host string) (addrs [] if order == hostLookupCgo { return cgoLookupIP(ctx, network, host) } - ips, _, err := r.goLookupIPCNAMEOrder(ctx, network, host, order, conf) + // Keep the resolver's addresses in their original order: the result + // may be shared by concurrent callers via singleflight, and is + // shuffled and sorted by RFC 6724 in lookupIPAddr. + ips, _, err := r.goLookupIPCNAME(ctx, network, host, order, conf) return ips, err }