net: don't return DNS query results including the second best records unconditionally

This change prevents DNS query results using domain search list
overtaking results not using the list unconditionally, which only
happens when using builtin DNS stub resolver.

The previous internal lookup function lookup is split into lookup and
goLookupIPOrder for iteration over a set of names: FQDN or absolute
FQDN, with domain label suffixes in search list, without domain label
suffixes, and for concurrent A and AAAA record queries.

Fixes #11081.

Change-Id: I9ff0640f69276e372d97e709b149ed5b153e8601
Reviewed-on: https://go-review.googlesource.com/10836Reviewed-by: Russ Cox <rsc@golang.org>

src/net/dnsclient.go

@@ -41,7 +41,7 @@ func answer(name, server string, dns *dnsMsg, qtype uint16) (cname string, addrs
 	addrs = make([]dnsRR, 0, len(dns.answer))
 
 	if dns.rcode == dnsRcodeNameError && dns.recursion_available {
-		return "", nil, &DNSError{Err: errNoSuchHost.Error(), Name: name}
+		return "", nil, &DNSError{Err: errNoSuchHost.Error(), Name: name, Server: server}
 	}
 	if dns.rcode != dnsRcodeSuccess {
 		// None of the error codes make sense

src/net/dnsclient_unix.go

@@ -185,9 +185,9 @@ func tryOneName(cfg *dnsConfig, name string, qtype uint16) (string, []dnsRR, err
 				}
 				continue
 			}
-			cname, addrs, err := answer(name, server, msg, qtype)
-			if err == nil || err.(*DNSError).Err == errNoSuchHost.Error() {
-				return cname, addrs, err
+			cname, rrs, err := answer(name, server, msg, qtype)
+			if err == nil || msg.rcode == dnsRcodeSuccess || msg.rcode == dnsRcodeNameError && msg.recursion_available {
+				return cname, rrs, err
 			}
 			lastErr = err
 		}
@@ -299,59 +299,55 @@ func (conf *resolverConfig) releaseSema() {
 
 func lookup(name string, qtype uint16) (cname string, rrs []dnsRR, err error) {
 	if !isDomainName(name) {
-		return name, nil, &DNSError{Err: "invalid domain name", Name: name}
+		return "", nil, &DNSError{Err: "invalid domain name", Name: name}
 	}
-
 	resolvConf.tryUpdate("/etc/resolv.conf")
 	resolvConf.mu.RLock()
-	resolv := resolvConf.dnsConfig
+	conf := resolvConf.dnsConfig
 	resolvConf.mu.RUnlock()
-
-	// If name is rooted (trailing dot) or has enough dots,
-	// try it by itself first.
-	rooted := len(name) > 0 && name[len(name)-1] == '.'
-	if rooted || count(name, '.') >= resolv.ndots {
-		rname := name
-		if !rooted {
-			rname += "."
-		}
-		// Can try as ordinary name.
-		cname, rrs, err = tryOneName(resolv, rname, qtype)
-		if rooted || err == nil {
-			return
-		}
-	}
-
-	// Otherwise, try suffixes.
-	for _, suffix := range resolv.search {
-		rname := name + "." + suffix
-		if rname[len(rname)-1] != '.' {
-			rname += "."
-		}
-		cname, rrs, err = tryOneName(resolv, rname, qtype)
+	for _, fqdn := range conf.nameList(name) {
+		cname, rrs, err = tryOneName(conf, fqdn, qtype)
 		if err == nil {
-			return
+			break
 		}
 	}
-
-	// Last ditch effort: try unsuffixed only if we haven't already,
-	// that is, name is not rooted and has less than ndots dots.
-	if count(name, '.') < resolv.ndots {
-		cname, rrs, err = tryOneName(resolv, name+".", qtype)
-		if err == nil {
-			return
-		}
-	}
-
-	if e, ok := err.(*DNSError); ok {
+	if err, ok := err.(*DNSError); ok {
 		// Show original name passed to lookup, not suffixed one.
 		// In general we might have tried many suffixes; showing
 		// just one is misleading. See also golang.org/issue/6324.
-		e.Name = name
+		err.Name = name
 	}
 	return
 }
 
+// nameList returns a list of names for sequential DNS queries.
+func (conf *dnsConfig) nameList(name string) []string {
+	// If name is rooted (trailing dot), try only that name.
+	rooted := len(name) > 0 && name[len(name)-1] == '.'
+	if rooted {
+		return []string{name}
+	}
+	// Build list of search choices.
+	names := make([]string, 0, 1+len(conf.search))
+	// If name has enough dots, try unsuffixed first.
+	if count(name, '.') >= conf.ndots {
+		names = append(names, name+".")
+	}
+	// Try suffixes.
+	for _, suffix := range conf.search {
+		suffixed := name + "." + suffix
+		if suffixed[len(suffixed)-1] != '.' {
+			suffixed += "."
+		}
+		names = append(names, suffixed)
+	}
+	// Try unsuffixed, if not tried first above.
+	if count(name, '.') < conf.ndots {
+		names = append(names, name+".")
+	}
+	return names
+}
+
 // hostLookupOrder specifies the order of LookupHost lookup strategies.
 // It is basically a simplified representation of nsswitch.conf.
 // "files" means /etc/hosts.
@@ -436,27 +432,44 @@ func goLookupIPOrder(name string, order hostLookupOrder) (addrs []IPAddr, err er
 			return addrs, nil
 		}
 	}
+	if !isDomainName(name) {
+		return nil, &DNSError{Err: "invalid domain name", Name: name}
+	}
+	resolvConf.tryUpdate("/etc/resolv.conf")
+	resolvConf.mu.RLock()
+	conf := resolvConf.dnsConfig
+	resolvConf.mu.RUnlock()
 	type racer struct {
-		qtype uint16
-		rrs   []dnsRR
+		rrs []dnsRR
 		error
 	}
 	lane := make(chan racer, 1)
 	qtypes := [...]uint16{dnsTypeA, dnsTypeAAAA}
-	for _, qtype := range qtypes {
-		go func(qtype uint16) {
-			_, rrs, err := lookup(name, qtype)
-			lane <- racer{qtype, rrs, err}
-		}(qtype)
-	}
 	var lastErr error
-	for range qtypes {
-		racer := <-lane
-		if racer.error != nil {
-			lastErr = racer.error
-			continue
+	for _, fqdn := range conf.nameList(name) {
+		for _, qtype := range qtypes {
+			go func(qtype uint16) {
+				_, rrs, err := tryOneName(conf, fqdn, qtype)
+				lane <- racer{rrs, err}
+			}(qtype)
+		}
+		for range qtypes {
+			racer := <-lane
+			if racer.error != nil {
+				lastErr = racer.error
+				continue
+			}
+			addrs = append(addrs, addrRecordList(racer.rrs)...)
+		}
+		if len(addrs) > 0 {
+			break
 		}
-		addrs = append(addrs, addrRecordList(racer.rrs)...)
+	}
+	if lastErr, ok := lastErr.(*DNSError); ok {
+		// Show original name passed to lookup, not suffixed one.
+		// In general we might have tried many suffixes; showing
+		// just one is misleading. See also golang.org/issue/6324.
+		lastErr.Name = name
 	}
 	sortByRFC6724(addrs)
 	if len(addrs) == 0 {

src/net/dnsclient_unix_test.go

@@ -224,6 +224,160 @@ func TestUpdateResolvConf(t *testing.T) {
 	}
 }
 
+var goLookupIPWithResolverConfigTests = []struct {
+	name  string
+	lines []string // resolver configuration lines
+	error
+	a, aaaa bool // whether response contains A, AAAA-record
+}{
+	// no records, transport timeout
+	{
+		"jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j",
+		[]string{
+			"options timeout:1 attempts:1",
+			"nameserver 255.255.255.255", // please forgive us for abuse of limited broadcast address
+		},
+		&DNSError{Name: "jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j", Server: "255.255.255.255:53", IsTimeout: true},
+		false, false,
+	},
+
+	// no records, non-existent domain
+	{
+		"jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j",
+		[]string{
+			"options timeout:3 attempts:1",
+			"nameserver 8.8.8.8",
+		},
+		&DNSError{Name: "jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j", Server: "8.8.8.8:53", IsTimeout: false},
+		false, false,
+	},
+
+	// a few A records, no AAAA records
+	{
+		"ipv4.google.com.",
+		[]string{
+			"nameserver 8.8.8.8",
+			"nameserver 2001:4860:4860::8888",
+		},
+		nil,
+		true, false,
+	},
+	{
+		"ipv4.google.com",
+		[]string{
+			"domain golang.org",
+			"nameserver 2001:4860:4860::8888",
+			"nameserver 8.8.8.8",
+		},
+		nil,
+		true, false,
+	},
+	{
+		"ipv4.google.com",
+		[]string{
+			"search x.golang.org y.golang.org",
+			"nameserver 2001:4860:4860::8888",
+			"nameserver 8.8.8.8",
+		},
+		nil,
+		true, false,
+	},
+
+	// no A records, a few AAAA records
+	{
+		"ipv6.google.com.",
+		[]string{
+			"nameserver 2001:4860:4860::8888",
+			"nameserver 8.8.8.8",
+		},
+		nil,
+		false, true,
+	},
+	{
+		"ipv6.google.com",
+		[]string{
+			"domain golang.org",
+			"nameserver 8.8.8.8",
+			"nameserver 2001:4860:4860::8888",
+		},
+		nil,
+		false, true,
+	},
+	{
+		"ipv6.google.com",
+		[]string{
+			"search x.golang.org y.golang.org",
+			"nameserver 8.8.8.8",
+			"nameserver 2001:4860:4860::8888",
+		},
+		nil,
+		false, true,
+	},
+
+	// both A and AAAA records
+	{
+		"hostname.as112.net", // see RFC 7534
+		[]string{
+			"domain golang.org",
+			"nameserver 2001:4860:4860::8888",
+			"nameserver 8.8.8.8",
+		},
+		nil,
+		true, true,
+	},
+	{
+		"hostname.as112.net", // see RFC 7534
+		[]string{
+			"search x.golang.org y.golang.org",
+			"nameserver 2001:4860:4860::8888",
+			"nameserver 8.8.8.8",
+		},
+		nil,
+		true, true,
+	},
+}
+
+func TestGoLookupIPWithResolverConfig(t *testing.T) {
+	if testing.Short() || !*testExternal {
+		t.Skip("avoid external network")
+	}
+
+	conf, err := newResolvConfTest()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conf.teardown()
+
+	for _, tt := range goLookupIPWithResolverConfigTests {
+		if err := conf.writeAndUpdate(tt.lines); err != nil {
+			t.Error(err)
+			continue
+		}
+		conf.tryUpdate(conf.path)
+		addrs, err := goLookupIP(tt.name)
+		if err != nil {
+			if err, ok := err.(*DNSError); !ok || (err.Name != tt.error.(*DNSError).Name || err.Server != tt.error.(*DNSError).Server || err.IsTimeout != tt.error.(*DNSError).IsTimeout) {
+				t.Errorf("got %v; want %v", err, tt.error)
+			}
+			continue
+		}
+		if len(addrs) == 0 {
+			t.Errorf("no records for %s", tt.name)
+		}
+		if !tt.a && !tt.aaaa && len(addrs) > 0 {
+			t.Errorf("unexpected %v for %s", addrs, tt.name)
+		}
+		for _, addr := range addrs {
+			if !tt.a && addr.IP.To4() != nil {
+				t.Errorf("got %v; must not be IPv4 address", addr)
+			}
+			if !tt.aaaa && addr.IP.To16() != nil && addr.IP.To4() == nil {
+				t.Errorf("got %v; must not be IPv6 address", addr)
+			}
+		}
+	}
+}
+
 func BenchmarkGoLookupIP(b *testing.B) {
 	testHookUninstaller.Do(uninstallTestHooks)