Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
philpennock opened this issue Mar 9, 2018 · 4 comments
Closed

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

philpennock opened this issue Mar 9, 2018 · 4 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@philpennock
Copy link

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
Copy link
Contributor

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

@as
Copy link
Contributor

as commented Mar 10, 2018

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
Copy link
Member

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
}

@odeke-em odeke-em modified the milestones: Go1.10.1, Go1.11 Mar 11, 2018
@odeke-em odeke-em added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 11, 2018
@gopherbot
Copy link

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

@ghost ghost mentioned this issue Jun 25, 2018
Closed
@golang golang locked and limited conversation to collaborators Mar 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
5 participants
@philpennock @fraenkel @odeke-em @as @gopherbot