From 705830e2b77cafc95e8e890bbafc4a57c37f8575 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 5 Jun 2019 12:35:29 -0400 Subject: [PATCH] net: fix non-cgo macOS resolver code This code was added in April in CL 166297, for #12524. This CL fixes the following problems in the code: - The test for failure in the assembly stubs checked for 64-bit -1 instead of 32-bit -1 to decide to fetch errno. - These C routines (res_init and res_search) don't set errno anyway, so the Go code using errno to decide success is incorrect. (The routines set h_errno, which is a racy global variable that can't safely be consulted, storing values in a different error space.) - The Go call passed res_search a non-NUL-terminated name. - The C res_search rejects calls asking for TypeALL as opposed to more specific answers like TypeA/TypeAAAA/TypeCNAME, breaking cgoLookupHost in all cases and cgoLookupIP except with IP-version-specific networks. - The DNS response packet was parsed twice, once with msg.Unpack (discarded), and once with the lower-level dnsmessage.Parser. The Parser loop was missing a call to p.SkipAllQuestions, with the result that no DNS response packet would ever parse successfully. - The parsing of the DNS response answers, if reached, behaved as if that the AResource and AAAAResource record contained textual IP addresses, while in fact they contain binary ones. The calls to parseIPv4 and parseIPv6 therefore would always returns nil, so that no useful result would be returned from the resolver. With these fixes, cgoLookupIP can correctly resolve google.com and return both the A and AAAA addresses. Even after fixing all these things, TestGoLookupIP still fails, because it is testing that in non-cgo builds the cgo stubs correctly report "I can't handle the lookup", and as written the code intentionally violates that expectation. This CL adds new direct tests of the pseudo-cgo routines. The direct IP address lookups succeed, but the CNAME query causes res_search to hang, and the PTR query fails unconditionally (a trivial C program confirms these behaviors are due to res_search itself). Traditionally, res_search is only intended for single-threaded use. It is unclear whether this one is safe for use from multiple goroutines. If you run net.test under lldb, that causes syslog messages to be printed to standard error suggesting double-free bugs: 2019-06-05 19:52:43.505246-0400 net.test[6256:6831076] dnssd_clientstub DNSServiceRefDeallocate called with invalid DNSServiceRef 0x5c000f0 FFFFFFFF DDDDDDDD 2019-06-05 19:52:43.505274-0400 net.test[6256:6831076] dnssd_clientstub DNSServiceRefDeallocate called with invalid DNSServiceRef 0x5c000f0 FFFFFFFF DDDDDDDD 2019-06-05 19:52:43.505303-0400 net.test[6256:6831076] dnssd_clientstub DNSServiceRefDeallocate called with invalid DNSServiceRef 0x5c000f0 FFFFFFFF DDDDDDDD 2019-06-05 19:52:43.505329-0400 net.test[6256:6831076] dnssd_clientstub DNSServiceRefDeallocate called with invalid DNSServiceRef 0x5c000f0 FFFFFFFF DDDDDDDD This res_search is from libsystem_info; a normal C program would get res_search (#defined to res_9_search) from libresolv instead. It is unclear what the relation between the two is. Issue #12524 was about supporting the /etc/resolver directory tree, but only libresolv contains code for that; libsystem_info does not. So this code probably does not enable use of /etc/resolver. In short: - Before this CL, the code clearly had never run successfully. - The code appears not to improve upon the usual non-cgo fallback. - The code carries with it no tests of improved behavior. - The code breaks existing tests. - Calling res_search does not work for PTR/CNAME queries, so the code breaks existing behavior, even after this CL. - It's unclear whether res_search is safe to call from multiple threads. - It's unclear whether res_search is used by any other macOS programs. Given this, it probably makes sense to delete this code rather than rejigger the test. This CL fixes the code first, so that there is a working copy to bring back later if we find out that it really is necessary. For #31705. Change-Id: Id2e11e8ade43098b0f90dd4d16a62ca86a7a244a Reviewed-on: https://go-review.googlesource.com/c/go/+/180842 Run-TryBot: Russ Cox Reviewed-by: Keith Randall --- src/net/cgo_darwin_stub.go | 183 +++++++++++++++++++------------- src/net/cgo_darwin_stub_test.go | 80 ++++++++++++++ 2 files changed, 188 insertions(+), 75 deletions(-) create mode 100644 src/net/cgo_darwin_stub_test.go diff --git a/src/net/cgo_darwin_stub.go b/src/net/cgo_darwin_stub.go index 544df7fd6c1b0b..fc50809d40400e 100644 --- a/src/net/cgo_darwin_stub.go +++ b/src/net/cgo_darwin_stub.go @@ -2,6 +2,33 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +// This file is intended to be used in non-cgo builds of darwin binaries, +// in particular when cross-compiling a darwin binary from a non-darwin machine. +// All OS calls on darwin have to be done via C libraries, and this code makes such +// calls with the runtime's help. (It is a C call but does not require the cgo tool to +// be compiled, and as such it is possible to build even when cross-compiling.) +// +// The specific C library calls are to res_init and res_search from /usr/lib/system/libsystem_info.dylib. +// Unfortunately, an ordinary C program calling these names would actually end up with +// res_9_init and res_9_search from /usr/lib/libresolv.dylib, not libsystem_info. +// It may well be that the libsystem_info routines are completely unused on macOS systems +// except for this code. At the least, they have the following problems: +// +// - TypeALL requests do not work, so if we want both IPv4 and IPv6 addresses, +// we have to do two requests, one for TypeA and one for TypeAAAA. +// - TypeCNAME requests hang indefinitely. +// - TypePTR requests fail unconditionally. +// - Detailed error information is stored in the global h_errno value, +// which cannot be accessed safely (it is not per-thread like errno). +// - The routines may not be safe to call from multiple threads. +// If you run net.test under lldb, that emits syslog prints to stderr +// that suggest double-free problems. (If not running under lldb, +// it is unclear where the syslog prints go, if anywhere.) +// +// This code is marked for deletion. If it is to be revived, it should be changed to use +// res_9_init and res_9_search from libresolv and special care should be paid to +// error detail and thread safety. + // +build !netgo,!cgo // +build darwin @@ -22,13 +49,21 @@ func (eai addrinfoErrno) Temporary() bool { return false } func (eai addrinfoErrno) Timeout() bool { return false } func cgoLookupHost(ctx context.Context, name string) (addrs []string, err error, completed bool) { - resources, err := resolverGetResources(ctx, name, int32(dnsmessage.TypeALL), int32(dnsmessage.ClassINET)) - if err != nil { - return - } - addrs, err = parseHostsFromResources(resources) - if err != nil { - return + // The 4-suffix indicates IPv4, TypeA lookups. + // The 6-suffix indicates IPv6, TypeAAAA lookups. + // If resSearch is updated to call the libresolv res_9_search (see comment at top of file), + // it may be possible to make one call for TypeALL + // and get both address kinds out. + r4, err4 := resSearch(ctx, name, int32(dnsmessage.TypeA), int32(dnsmessage.ClassINET)) + if err4 == nil { + addrs, err4 = appendHostsFromResources(addrs, r4) + } + r6, err6 := resSearch(ctx, name, int32(dnsmessage.TypeAAAA), int32(dnsmessage.ClassINET)) + if err6 == nil { + addrs, err6 = appendHostsFromResources(addrs, r6) + } + if err4 != nil && err6 != nil { + return nil, err4, false } return addrs, nil, true } @@ -39,30 +74,35 @@ func cgoLookupPort(ctx context.Context, network, service string) (port int, err } func cgoLookupIP(ctx context.Context, network, name string) (addrs []IPAddr, err error, completed bool) { - - var resources []dnsmessage.Resource - switch ipVersion(network) { - case '4': - resources, err = resolverGetResources(ctx, name, int32(dnsmessage.TypeA), int32(dnsmessage.ClassINET)) - case '6': - resources, err = resolverGetResources(ctx, name, int32(dnsmessage.TypeAAAA), int32(dnsmessage.ClassINET)) - default: - resources, err = resolverGetResources(ctx, name, int32(dnsmessage.TypeALL), int32(dnsmessage.ClassINET)) + // The 4-suffix indicates IPv4, TypeA lookups. + // The 6-suffix indicates IPv6, TypeAAAA lookups. + // If resSearch is updated to call the libresolv res_9_search (see comment at top of file), + // it may be possible to make one call for TypeALL (when vers != '6' and vers != '4') + // and get both address kinds out. + var r4, r6 []dnsmessage.Resource + var err4, err6 error + vers := ipVersion(network) + if vers != '6' { + r4, err4 = resSearch(ctx, name, int32(dnsmessage.TypeA), int32(dnsmessage.ClassINET)) + if err4 == nil { + addrs, err4 = appendIPsFromResources(addrs, r4) + } } - if err != nil { - return + if vers != '4' { + r6, err6 = resSearch(ctx, name, int32(dnsmessage.TypeAAAA), int32(dnsmessage.ClassINET)) + if err6 == nil { + addrs, err6 = appendIPsFromResources(addrs, r6) + } } - - addrs, err = parseIPsFromResources(resources) - if err != nil { - return + if err4 != nil && err6 != nil { + return nil, err4, false } return addrs, nil, true } func cgoLookupCNAME(ctx context.Context, name string) (cname string, err error, completed bool) { - resources, err := resolverGetResources(ctx, name, int32(dnsmessage.TypeCNAME), int32(dnsmessage.ClassINET)) + resources, err := resSearch(ctx, name, int32(dnsmessage.TypeCNAME), int32(dnsmessage.ClassINET)) if err != nil { return } @@ -74,7 +114,7 @@ func cgoLookupCNAME(ctx context.Context, name string) (cname string, err error, } func cgoLookupPTR(ctx context.Context, addr string) (ptrs []string, err error, completed bool) { - resources, err := resolverGetResources(ctx, addr, int32(dnsmessage.TypePTR), int32(dnsmessage.ClassINET)) + resources, err := resSearch(ctx, addr, int32(dnsmessage.TypePTR), int32(dnsmessage.ClassINET)) if err != nil { return } @@ -86,69 +126,66 @@ func cgoLookupPTR(ctx context.Context, addr string) (ptrs []string, err error, c } var ( - resInitOnce sync.Once - errCode int32 + resInitOnce sync.Once + resInitResult int32 ) -// resolverGetResources will make a call to the 'res_search' routine in libSystem +// resSearch will make a call to the 'res_search' routine in libSystem // and parse the output as a slice of resource resources which can then be parsed -func resolverGetResources(ctx context.Context, hostname string, rtype, class int32) ([]dnsmessage.Resource, error) { - +func resSearch(ctx context.Context, hostname string, rtype, class int32) ([]dnsmessage.Resource, error) { + // We have to use res_init and res_search, but these do not set errno on failure. + // (They set h_errno, which is a global int shared by all threads and therefore + // racy to use.) + // https://opensource.apple.com/source/Libinfo/Libinfo-517.200.9/dns.subproj/res_query.c.auto.html resInitOnce.Do(func() { - errCode = res_init() + resInitResult = res_init() }) - if errCode < 0 { - return nil, errors.New("could not initialize name resolver data") - } - - var byteHostname = []byte(hostname) - var responseBuffer [512]byte - var size int32 - - size, errCode = res_search(&byteHostname[0], class, rtype, &responseBuffer[0], int32(len(responseBuffer))) - if errCode != 0 { - return nil, errors.New("could not complete domain resolution return code " + string(errCode)) - } - if size == 0 { - return nil, errors.New("received empty response") - } - - var msg dnsmessage.Message - err := msg.Unpack(responseBuffer[:]) - if err != nil { + if resInitResult < 0 { + return nil, errors.New("res_init failure") + } + + // res_search does not set errno. + // It returns the size of the DNS response packet. + // But if the DNS response packet contains failure-like response codes, + // res_search returns -1 even though it has copied the packet into buf, + // giving us no way to find out how big the packet is. + // For now, we are willing to take res_search's word that there's nothing + // useful in the response, even though there *is* a response. + name := make([]byte, len(hostname)+1) // +1 for NUL at end for C + copy(name, hostname) + var buf [1024]byte + size, _ := res_search(&name[0], class, rtype, &buf[0], int32(len(buf))) + if size <= 0 { + return nil, errors.New("res_search failure") + } + + var p dnsmessage.Parser + if _, err := p.Start(buf[:size]); err != nil { return nil, err } - - var dnsParser dnsmessage.Parser - if _, err := dnsParser.Start(responseBuffer[:]); err != nil { + p.SkipAllQuestions() + resources, err := p.AllAnswers() + if err != nil { return nil, err } - - var resources []dnsmessage.Resource - for { - r, err := dnsParser.Answer() - if err == dnsmessage.ErrSectionDone { - break - } - if err != nil { - return nil, err - } - resources = append(resources, r) - } return resources, nil } -func parseHostsFromResources(resources []dnsmessage.Resource) ([]string, error) { - var answers []string +func copyBytes(x []byte) []byte { + y := make([]byte, len(x)) + copy(y, x) + return y +} +func appendHostsFromResources(answers []string, resources []dnsmessage.Resource) ([]string, error) { for i := range resources { switch resources[i].Header.Type { case dnsmessage.TypeA: b := resources[i].Body.(*dnsmessage.AResource) - answers = append(answers, string(b.A[:])) + answers = append(answers, IP(b.A[:]).String()) case dnsmessage.TypeAAAA: b := resources[i].Body.(*dnsmessage.AAAAResource) - answers = append(answers, string(b.AAAA[:])) + answers = append(answers, IP(b.AAAA[:]).String()) default: return nil, errors.New("could not parse an A or AAAA response from message buffer") } @@ -156,19 +193,15 @@ func parseHostsFromResources(resources []dnsmessage.Resource) ([]string, error) return answers, nil } -func parseIPsFromResources(resources []dnsmessage.Resource) ([]IPAddr, error) { - var answers []IPAddr - +func appendIPsFromResources(answers []IPAddr, resources []dnsmessage.Resource) ([]IPAddr, error) { for i := range resources { switch resources[i].Header.Type { case dnsmessage.TypeA: b := resources[i].Body.(*dnsmessage.AResource) - ip := parseIPv4(string(b.A[:])) - answers = append(answers, IPAddr{IP: ip}) + answers = append(answers, IPAddr{IP: IP(copyBytes(b.A[:]))}) case dnsmessage.TypeAAAA: b := resources[i].Body.(*dnsmessage.AAAAResource) - ip, zone := parseIPv6Zone(string(b.AAAA[:])) - answers = append(answers, IPAddr{IP: ip, Zone: zone}) + answers = append(answers, IPAddr{IP: IP(copyBytes(b.AAAA[:]))}) default: return nil, errors.New("could not parse an A or AAAA response from message buffer") } diff --git a/src/net/cgo_darwin_stub_test.go b/src/net/cgo_darwin_stub_test.go new file mode 100644 index 00000000000000..f694e2a0cb77b0 --- /dev/null +++ b/src/net/cgo_darwin_stub_test.go @@ -0,0 +1,80 @@ +// Copyright 2019 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build !netgo,!cgo +// +build darwin + +package net + +import ( + "context" + "strings" + "testing" +) + +func TestPseudoCgoLookupHost(t *testing.T) { + addrs, err, ok := cgoLookupHost(context.Background(), "google.com") + t.Logf("cgoLookupHost google.com: %v %v %v", addrs, err, ok) + if !ok { + t.Fatal("cgoLookupHost ok=false") + } + if err != nil { + t.Fatalf("cgoLookupHost: %v", err) + } + // cgoLookupHost need not return IPv4 before IPv6 in general, + // but for the current implementation it does. + // If that changes, this test will need updating. + if len(addrs) < 1 || strings.Count(addrs[0], ".") != 3 || !strings.Contains(addrs[len(addrs)-1], "::") { + t.Fatalf("cgoLookupHost google.com = %v, want IPv4 and IPv6", addrs) + } +} + +func TestPseudoCgoLookupIP(t *testing.T) { + ips, err, ok := cgoLookupIP(context.Background(), "ip", "google.com") + t.Logf("cgoLookupIP google.com: %v %v %v", ips, err, ok) + if !ok { + t.Fatal("cgoLookupIP ok=false") + } + if err != nil { + t.Fatalf("cgoLookupIP: %v", err) + } + // cgoLookupIP need not return IPv4 before IPv6 in general, + // but for the current implementation it does. + // If that changes, this test will need updating. + if len(ips) < 1 || len(ips[0].IP) != 4 || len(ips[len(ips)-1].IP) != 16 { + t.Fatalf("cgoLookupIP google.com = %v, want IPv4 and IPv6", ips) + } +} + +func TestPseudoCgoLookupCNAME(t *testing.T) { + t.Skip("res_search on macOS hangs in TypeCNAME queries (even in plain C programs)") + + cname, err, ok := cgoLookupCNAME(context.Background(), "redirect.swtch.com") + t.Logf("cgoLookupCNAME redirect.swtch.com: %v %v %v", cname, err, ok) + if !ok { + t.Fatal("cgoLookupCNAME ok=false") + } + if err != nil { + t.Fatalf("cgoLookupCNAME: %v", err) + } + if !strings.HasSuffix(cname, ".com") { + t.Fatalf("cgoLookupCNAME redirect.swtch.com = %v, want *.com", cname) + } +} + +func TestPseudoCgoLookupPTR(t *testing.T) { + t.Skip("res_search on macOS does not support TypePTR") + + ptrs, err, ok := cgoLookupPTR(context.Background(), "8.8.8.8") + t.Logf("cgoLookupPTR 8.8.8.8: %v %v %v", ptrs, err, ok) + if !ok { + t.Fatal("cgoLookupPTR ok=false") + } + if err != nil { + t.Fatalf("cgoLookupPTR: %v", err) + } + if len(ptrs) < 1 || ptrs[0] != "google-public-dns-a.google.com" { + t.Fatalf("cgoLookupPTR = %v, want google-public-dns-a.google.com", ptrs) + } +}