Skip to content

Commit

Permalink
net: fix non-cgo macOS resolver code
Browse files Browse the repository at this point in the history 
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 <rsc@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
  • Loading branch information
@rsc
rsc committed Jun 6, 2019
1 parent 7d65e3a commit 705830e
Show file tree
Hide file tree
Showing 2 changed files with 188 additions and 75 deletions.
183 changes: 108 additions & 75 deletions src/net/cgo_darwin_stub.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -86,89 +126,82 @@ 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")
}
}
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")
}
Expand Down
80 changes: 80 additions & 0 deletions src/net/cgo_darwin_stub_test.go
View file
Original file line number Diff line number Diff line change
@@ -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)
}
}

0 comments on commit 705830e

Please sign in to comment.