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

net: in cgoLookupIPCNAME(), let C.getaddrinfo() get a hint about network family #25947

Closed
albertjin opened this issue Jun 18, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@albertjin
Copy link

commented Jun 18, 2018

I am talking about the following function in src/net/cgo_unix.go,

func cgoLookupIPCNAME(name string) (addrs []IPAddr, cname string, err error) {
	acquireThread()
	defer releaseThread()

	var hints C.struct_addrinfo
	//TODO: assign a value to hints.ai_family based on network family
	hints.ai_flags = cgoAddrInfoFlags
	hints.ai_socktype = C.SOCK_STREAM

	h := make([]byte, len(name)+1)
	copy(h, name)
	var res *C.struct_addrinfo
	gerrno, err := C.getaddrinfo((*C.char)(unsafe.Pointer(&h[0])), nil, &hints, &res)
	if gerrno != 0 {
        // ...

Under macOS, getaddrinfo() will block for 5 seconds when my router drops all UDP packets of AAAA dns requests. But the requirement is barely about connecting to an IPv4 based server and no AAAA address is needed.

Well, I do reconfigure my route to reply all AAAA queries with responses of an empty record set and the problem is solved. A fix of the library internals will be useful anyway.

@bradfitz bradfitz added this to the Go1.12 milestone Jun 18, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

Seems reasonable, if it doesn't require too much plumbing inside the net package to pass that context down to cgoLookupIPCNAME, and if the result isn't coalesced/cached between different contexts.

Somebody is welcome to investigate & propose a CL.

/cc @ianlancetaylor

@ekalinin

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2018

Hey @albertjin

But the requirement is barely about connecting to an IPv4 based server and no AAAA address is needed.

Could you provide some example to reproduce the issue, please? As far as I can see LookupCNAME (or Resolver.LookupCNAME) does not have any args for network or other info which could be useful.

@albertjin

This comment has been minimized.

Copy link
Author

commented Jun 19, 2018

Hi @ekalinin

For a quick drive, use DialContext to connect a server.

    dialer := &net.Dialer{}
    conn, err := dialer.DialContext(context.Background(), "tcp4", "golang.org:443")

We want to dial to a "tcp4" network while cgoLookupIPCNAME() does not get this hint and pass it to C. getaddrinfo().

@ekalinin

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

@albertjin Thanks!

Here's approximate calls stack:

Dialer.DialContext                      // has network
    Resolver.resolveAddrList            // has network
        Resolver.internetAddrList       // has network
            Resolver.LookupIPAddr       // no network
                singleflight.Group.DoChan
                    testHookLookupIP
                        Resolver.lookupIP
                            cgoLookupIP
                                cgoLookupIPCNAME

@bradfitz Resolver.LookupIPAddr is a public method. Which of the options would be better? Create a new method (with a new argument network) or update an existing one (in this case this method will lose backward compatibility)?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

We can not change the signature of an exported method, as that would break the Go 1 compatibility guarantee. This would have to be done using a new, unexported, method. Presumably LookupIPAddr could be renamed and replace with a stub that called the new method with an empty string.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 20, 2018

Change https://golang.org/cl/120215 mentions this issue: net: add hit for C.getaddrinfo in cgoLookupIPCNAME

@ekalinin

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

Tested by simple:

➜ cat dial-test.go
package main

import "fmt"
import "net"

func main() {
    _, err := net.Dial("tcp4", "golang.org:443")
    if err != nil {
        fmt.Println(err)
        return
    }
    fmt.Println("Done.")
}

Results:

#
# with fix
#
➜ sudo killall -HUP mDNSResponder;sudo killall mDNSResponderHelper;sudo dscacheutil -flushcache
➜ sudo tcpdump -lvi any "udp port 53"
01:31:23.767699 IP (tos 0x0, ttl 255, id 28637, offset 0, flags [none], proto UDP (17), length 56)
    myosx.56516 > router.asus.com.domain: 43089+ A? golang.org. (28)
01:31:23.769347 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 72)
    router.asus.com.domain > myosx.56516: 43089 1/0/0 golang.org. A 173.194.222.141 (44)

#
# ➜ go version
# go version go1.10.3 darwin/amd64
#
➜ sudo killall -HUP mDNSResponder;sudo killall mDNSResponderHelper;sudo dscacheutil -flushcache
➜ sudo tcpdump -lvi any "udp port 53"
01:31:58.597495 IP (tos 0x0, ttl 255, id 49249, offset 0, flags [none], proto UDP (17), length 56)
    myosx.49561 > router.asus.com.domain: 24596+ A? golang.org. (28)
01:31:58.597852 IP (tos 0x0, ttl 255, id 23053, offset 0, flags [none], proto UDP (17), length 56)
    myosx.52472 > router.asus.com.domain: 52093+ AAAA? golang.org. (28)
01:31:58.600122 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 72)
    router.asus.com.domain > myosx.49561: 24596 1/0/0 golang.org. A 173.194.222.141 (44)
01:31:58.622543 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto UDP (17), length 84)
    router.asus.com.domain > myosx.52472: 52093 1/0/0 golang.org. AAAA 2a00:1450:4010:c0b::8d (56)

@gopherbot gopherbot closed this in c659be4 Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.