net: nil *Resolver not equivalent to zero resolver, despite doc

[philpennock]

What version of Go are you using (go version)?

go version go1.10 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/pdp/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/pdp/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vs/7q9nz5955_z7b7zxfz437zrm0000gn/T/go-build515078480=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Tested the assertion at https://golang.org/pkg/net/#Resolver that:

A nil *Resolver is equivalent to a zero Resolver.

https://play.golang.com/p/B40hcxy_Uuu

In the Go sandbox, it errors out because of the network restrictions. Run locally, this panics:

panic: runtime error: invalid memory address or nil pointer dereference

Swap the commenting-out of the r declaration to create a zero resolver, and the code runs perfectly.

What did you expect to see?

No difference in output between the two ways of declaring the resolver, because the documentation explicitly asserts the behavior of a nil *Resolver.

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x10bedd6]

goroutine 5 [running]:
net.(*Resolver).lookupIP(0x0, 0x112e2e0, 0xc4200180f0, 0x1119095, 0xe, 0x0, 0xc420001b00, 0xc420062040, 0x0, 0x0)
	/usr/local/go/src/net/lookup_unix.go:90 +0x26
net.(*Resolver).(net.lookupIP)-fm(0x112e2e0, 0xc4200180f0, 0x1119095, 0xe, 0x1028169, 0x8, 0xc420062040, 0x0, 0xc420035ea0)
	/usr/local/go/src/net/lookup.go:192 +0x56
net.glob..func10(0x112e2e0, 0xc4200180f0, 0xc420010370, 0x1119095, 0xe, 0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/hook.go:19 +0x52
net.(*Resolver).LookupIPAddr.func1(0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/lookup.go:200 +0xd8
internal/singleflight.(*Group).doCall(0x11bbdf0, 0xc42009e0f0, 0x1119095, 0xe, 0xc420090510)
	/usr/local/go/src/internal/singleflight/singleflight.go:95 +0x2e
created by internal/singleflight.(*Group).DoChan
	/usr/local/go/src/internal/singleflight/singleflight.go:88 +0x2d0
exit status 2

[fraenkel]

There are actually a few places which will panic since access to PreferGo or StrictErrors does not check if the resolver is nil.

[as]

Can't reproduce this exact example on go version go1.10 windows/amd64 (but is reproductible on darwin). The comment seems wrong though, or at least easier to fix than the code.

• There's already a net.DefaultResolver in stdlib so making your own I would have automatically assumed it would be required to be non-nil.

• http.Client (which has http.DefaultClient) doesn't guarantee that *http.Client is equal to it's zero value (and would probably crash and burn in the same manner).

/cc @bradfitz

[odeke-em]

Thank you for this catch @philpennock! In deed we made this promise but as @fraenkel noted, there are few places in which on UNIX like systems where we directly invoke fields on the *Resolver[1]

On other systems we don't even seem to be touching *Resolver[1]

Anyways this is just an oversight, we made that assertion ~1.5 years ago in September 2016, in this CL https://go-review.googlesource.com/29440 and commit 2bc5f12 but
changed the UNIX code up ~1.4 years ago in October 2016, in this CL https://go-review.googlesource.com/c/31734/ and commit 40d4be5.

I'd suggest perhaps:
a) Remove that documentation promise entirely as I think that at the time it was subjective by consensus(i.e. every other resolver had a method to use the default Resolver if it were nil)
b) Fix it for UNIX systems and then add a test to lock this behavior in. This way if any future edits change up the resolvers, the assertion test will panic and perhaps also clarify that we meant, Using a nil *Resolver will instead use the default Resolver because currently A nil *Resolver is equivalent to a zero Resolver. is too strong and the equivalence implied could be misunderstood as literally being equivalent like ==.

[1]

System	Comment	Code
Nacl	Noop	go/src/net/lookup_nacl.go Lines 18 to 20 in c15b7b2 func (*Resolver) lookupHost(ctx context.Context, host string) (addrs []string, err error) { return nil, syscall.ENOPROTOOPT }
Windows	Not even referenced in lookup code	go/src/net/lookup_windows.go Lines 79 to 136 in c15b7b2 func (r *Resolver) lookupIP(ctx context.Context, name string) ([]IPAddr, error) { // TODO(bradfitz,brainman): use ctx more. See TODO below. type ret struct { addrs []IPAddr err error } ch := make(chan ret, 1) go func() { acquireThread() defer releaseThread() hints := syscall.AddrinfoW{ Family: syscall.AF_UNSPEC, Socktype: syscall.SOCK_STREAM, Protocol: syscall.IPPROTO_IP, } var result *syscall.AddrinfoW e := syscall.GetAddrInfoW(syscall.StringToUTF16Ptr(name), nil, &hints, &result) if e != nil { ch <- ret{err: &DNSError{Err: winError("getaddrinfow", e).Error(), Name: name}} } defer syscall.FreeAddrInfoW(result) addrs := make([]IPAddr, 0, 5) for ; result != nil; result = result.Next { addr := unsafe.Pointer(result.Addr) switch result.Family { case syscall.AF_INET: a := (*syscall.RawSockaddrInet4)(addr).Addr addrs = append(addrs, IPAddr{IP: IPv4(a[0], a[1], a[2], a[3])}) case syscall.AF_INET6: a := (*syscall.RawSockaddrInet6)(addr).Addr zone := zoneCache.name(int((*syscall.RawSockaddrInet6)(addr).Scope_id)) addrs = append(addrs, IPAddr{IP: IP{a[0], a[1], a[2], a[3], a[4], a[5], a[6], a[7], a[8], a[9], a[10], a[11], a[12], a[13], a[14], a[15]}, Zone: zone}) default: ch <- ret{err: &DNSError{Err: syscall.EWINDOWS.Error(), Name: name}} } } ch <- ret{addrs: addrs} }() select { case r := <-ch: return r.addrs, r.err case <-ctx.Done(): // TODO(bradfitz,brainman): cancel the ongoing // GetAddrInfoW? It would require conditionally using // GetAddrInfoEx with lpOverlapped, which requires // Windows 8 or newer. I guess we'll need oldLookupIP, // newLookupIP, and newerLookUP. // // For now we just let it finish and write to the // buffered channel. return nil, &DNSError{ Name: name, Err: ctx.Err().Error(), IsTimeout: ctx.Err() == context.DeadlineExceeded, } } }
Plan9	Not even referenced in lookup method	go/src/net/lookup_plan9.go Lines 120 to 152 in c15b7b2 func (*Resolver) lookupHost(ctx context.Context, host string) (addrs []string, err error) { // Use netdir/cs instead of netdir/dns because cs knows about // host names in local network (e.g. from /lib/ndb/local) lines, err := queryCS(ctx, "net", host, "1") if err != nil { if stringsHasSuffix(err.Error(), "dns failure") { err = errNoSuchHost } return } loop: for _, line := range lines { f := getFields(line) if len(f) < 2 { continue } addr := f[1] if i := byteIndex(addr, '!'); i >= 0 { addr = addr[:i] // remove port } if ParseIP(addr) == nil { continue } // only return unique addresses for _, a := range addrs { if a == addr { continue loop } } addrs = append(addrs, addr) } return }
UNIX	Definitely, fields are invoked on it and will panic	go/src/net/lookup_unix.go Lines 77 to 103 in c15b7b2 func (r *Resolver) lookupHost(ctx context.Context, host string) (addrs []string, err error) { order := systemConf().hostLookupOrder(host) if !r.PreferGo && order == hostLookupCgo { if addrs, err, ok := cgoLookupHost(ctx, host); ok { return addrs, err } // cgo not available (or netgo); fall back to Go's DNS resolver order = hostLookupFilesDNS } return r.goLookupHostOrder(ctx, host, order) } func (r *Resolver) lookupIP(ctx context.Context, host string) (addrs []IPAddr, err error) { if r.PreferGo { return r.goLookupIP(ctx, host) } order := systemConf().hostLookupOrder(host) if order == hostLookupCgo { if addrs, err, ok := cgoLookupIP(ctx, host); ok { return addrs, err } // cgo not available (or netgo); fall back to Go's DNS resolver order = hostLookupFilesDNS } addrs, _, err = r.goLookupIPCNAMEOrder(ctx, host, order) return }

[gopherbot]

Change https://golang.org/cl/101315 mentions this issue: net: treat a nil *Resolver as a zero one, as documented