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: UDPAddr's (and possibly TCPAddr's) AddrPort returns addresses with the wrong IP version #53607

Open
ainar-g opened this issue Jun 29, 2022 · 13 comments
Labels
NeedsInvestigation

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Jun 29, 2022

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

$ go version
go version go1.18.3 linux/amd64

Does this issue reproduce with the latest release?

It does with go version devel go1.19-d3ffff2790 Tue Jun 28 13:01:41 2022 +0000 linux/amd64, haven't tried a more recent one.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ainar/.cache/go-build"
GOENV="/home/ainar/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ainar/go/pkg/mod"
GONOPROXY="REMOVED"
GONOSUMDB="REMOVED"
GOOS="linux"
GOPATH="/home/ainar/go"
GOPRIVATE="REMOVED"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/ainar/go/go1.18"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/ainar/go/go1.18/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
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=/tmp/go-build1722739114=/tmp/go-build -gno-record-gcc-switches"

What did you do?

c, err := net.ListenUDP("udp", &net.UDPAddr{
	IP:   net.IPv4zero,
	Port: 10000,
})
if err != nil {
	panic(err)
}

var buf [64]byte
_, raddr, err := c.ReadFromUDPAddrPort(buf[:])
if err != nil {
	panic(err)
}

fmt.Println(raddr)
fmt.Println("is4:", raddr.Addr().Is4())
fmt.Println("is6:", raddr.Addr().Is6())
fmt.Println("is4in6:", raddr.Addr().Is4In6())

_, oldRAddr, err := c.ReadFromUDP(buf[:])
if err != nil {
	panic(err)
}

raddr = oldRAddr.AddrPort()
fmt.Println(oldRAddr.Network())
fmt.Println(raddr)
fmt.Println("is4:", raddr.Addr().Is4())
fmt.Println("is6:", raddr.Addr().Is6())
fmt.Println("is4in6:", raddr.Addr().Is4In6())

https://go.dev/play/p/9qs4KxGRPjq

go run ./main.go
echo 'hello' | nc -4 -u -w 1 127.0.0.1 10000
echo 'world' | nc -4 -u -w 1 127.0.0.1 10000

Note: the requests are sent using -4 and an IPv4 address.

What did you expect to see?

127.0.0.1:58848
is4: true
is6: false
is4in6: false
udp
127.0.0.1:38564
is4: true
is6: false
is4in6: false

What did you see instead?

[::ffff:127.0.0.1]:58848
is4: false
is6: true
is4in6: true
udp
[::ffff:127.0.0.1]:38564
is4: false
is6: true
is4in6: true

I haven't tried the TCP ones, but looking at the code, they probably have the same issue.

@ZekeLu
Copy link

ZekeLu commented Jun 30, 2022

I think udp4 instead of udp should be specified if you expect to use IPv4.

- c, err := net.ListenUDP("udp", &net.UDPAddr{
+ c, err := net.ListenUDP("udp4", &net.UDPAddr{

The behavior is documented here:

go/src/net/ipsock_posix.go

Lines 73 to 112 in 17083a2

// favoriteAddrFamily returns the appropriate address family for the
// given network, laddr, raddr and mode.
//
// If mode indicates "listen" and laddr is a wildcard, we assume that
// the user wants to make a passive-open connection with a wildcard
// address family, both AF_INET and AF_INET6, and a wildcard address
// like the following:
//
// - A listen for a wildcard communication domain, "tcp" or
// "udp", with a wildcard address: If the platform supports
// both IPv6 and IPv4-mapped IPv6 communication capabilities,
// or does not support IPv4, we use a dual stack, AF_INET6 and
// IPV6_V6ONLY=0, wildcard address listen. The dual stack
// wildcard address listen may fall back to an IPv6-only,
// AF_INET6 and IPV6_V6ONLY=1, wildcard address listen.
// Otherwise we prefer an IPv4-only, AF_INET, wildcard address
// listen.
//
// - A listen for a wildcard communication domain, "tcp" or
// "udp", with an IPv4 wildcard address: same as above.
//
// - A listen for a wildcard communication domain, "tcp" or
// "udp", with an IPv6 wildcard address: same as above.
//
// - A listen for an IPv4 communication domain, "tcp4" or "udp4",
// with an IPv4 wildcard address: We use an IPv4-only, AF_INET,
// wildcard address listen.
//
// - A listen for an IPv6 communication domain, "tcp6" or "udp6",
// with an IPv6 wildcard address: We use an IPv6-only, AF_INET6
// and IPV6_V6ONLY=1, wildcard address listen.
//
// Otherwise guess: If the addresses are IPv4 then returns AF_INET,
// or else returns AF_INET6. It also returns a boolean value what
// designates IPV6_V6ONLY option.
//
// Note that the latest DragonFly BSD and OpenBSD kernels allow
// neither "net.inet6.ip6.v6only=1" change nor IPPROTO_IPV6 level
// IPV6_V6ONLY socket option setting.
func favoriteAddrFamily(network string, laddr, raddr sockaddr, mode string) (family int, ipv6only bool) {

@ainar-g
Copy link
Contributor Author

ainar-g commented Jun 30, 2022

I've known about the udp4 workaround, but I don't think that that is optimal for those who want to accept both IPv4 and IPv6 connections and get the correct address versions. With net.IP that wasn't an issue, since the mapped IPv4 addresses were essentially the same as the real, four-byte ones. That isn't the case with netip.Addr.

At the very least, there should be public documentation about that. Having the correct type of address would be better, but I don't know if that's feasible.

@ZekeLu
Copy link

ZekeLu commented Jun 30, 2022

With net.IP that wasn't an issue, since the mapped IPv4 addresses were essentially the same as the real, four-byte ones. That isn't the case with netip.Addr.

Can you elaborate this part? I don't understand why netip.Addr is different in this case.

@ainar-g
Copy link
Contributor Author

ainar-g commented Jun 30, 2022

net.ParseIP and similar functions return mapped forms for IPv4 addresses, so the common way to check whether a net.IP contains an IPv4 address was:

if ip4 := ip.To4(); ip4 != nil {
        // Use ip4 for IPv4 code.
} else {
        // Use ip for IPv6 code.
}

Which wasn't entirely correct, since there could be situations where one needs to make a distinction between the real IPv4 addresses and the mapped ones, but those seem to be fairly rare.

netip.Addr makes the distinction clearer with its Is4, but Is4 returns false for the mapped addresses, since there is Is4In6, which returns true for them. Thus, there is a lot of older code which assumes that all mapped addresses are actually just IPv4 addresses, because net.IP essentially has no other way to tell that, since ParseIP and friends always return mapped addresses. And if that code is then rewritten by simply replacing the ip4 != nil check with ip.Is4, which is a perfectly reasonable rewrite at a glance, that code is suddenly broken for addresses returned by the methods from the original post, because the addresses for connections coming over IPv4 returned by them aren't really IPv4 addresses any more.

@dr2chase dr2chase added the NeedsInvestigation label Jun 30, 2022
@dr2chase
Copy link
Contributor

dr2chase commented Jun 30, 2022

This is a duplicate of #53554, which has a proposed fix as well.

@dr2chase dr2chase closed this as not planned Jun 30, 2022
@ZekeLu
Copy link

ZekeLu commented Jun 30, 2022

@dr2chase I think the two issues are different. The proposed fix of #53554 won't affect this issue. IIUC, this issue is more about the documentation.

@ainar-g
Copy link
Contributor Author

ainar-g commented Jun 30, 2022

@dr2chase, I agree with ZekeLu in that this is a separate issue. And I also feel like there may possibly be other places where a naïve conversion could result in code breakage. Perhaps a blog post outlining the dos and don'ts of converting code from net.IP to netip.Addr would clarify the situation. Along with notes in the documentation, of course.

@dr2chase dr2chase reopened this Jun 30, 2022
@bradfitz
Copy link
Contributor

bradfitz commented Aug 3, 2022

Another example: https://go.dev/play/p/wVDZEuLFdRd

	ta := &net.TCPAddr{
		IP:   net.ParseIP("1.2.3.4"),
		Port: 80,
	}
	fmt.Println(ta.AddrPort())

results in:

[::ffff:1.2.3.4]:80

In retrospect, making https://pkg.go.dev/net/netip#AddrFromSlice not do the automatic unmapping was probably a mistake. I just converted a bunch of code from the original https://pkg.go.dev/inet.af/netaddr#FromStdIP which does do the unmapping and it was pretty tedious, having to sprinkle Unmap calls everywhere. But when the IP is inside an AddrPort like the example in this comment, then fixing them up's even more tedious.

Some options:

  • change a bunch of the net package to return 4-byte net.IP slices wherever safe & possible, to not confuse netip.AddrFromSlice (like https://go-review.googlesource.com/c/go/+/415580 and ParseIP)
  • change netip.AddrFromSlice to also Unmap. Notably, its API docs aren't very specific on which behavior it implements, so it's note a huge breaking change.
  • change the {TCP,UDP}Addr.AddrPort methods to Unmap at least, before the port wrapper

/cc @josharian @rsc @ianlancetaylor @maisem @crawshaw

bradfitz added a commit to tailscale/tailscale that referenced this issue Aug 3, 2022
With caveat golang/go#53607 (comment)
that then requires a new wrapper. But a simpler one at least.

Updates #5162

Change-Id: I0a5265065bfcd7f21e8dd65b2bd74cae90d76090
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit to tailscale/tailscale that referenced this issue Aug 3, 2022
With caveat golang/go#53607 (comment)
that then requires a new wrapper. But a simpler one at least.

Updates #5162

Change-Id: I0a5265065bfcd7f21e8dd65b2bd74cae90d76090
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@ainar-g
Copy link
Contributor Author

ainar-g commented Aug 3, 2022

@bradfitz, just to clarify, in your example the main culprit is probably net.ParseIP, which always returns a 16-byte form, not net.TCPAddr.AddrPort. If you use a “raw” 4-byte IP address or call To4(), it works as intended:

ta := &net.TCPAddr{
	IP:   net.ParseIP("1.2.3.4").To4(),
	Port: 80,
}
fmt.Println(ta.AddrPort())

ta = &net.TCPAddr{
	IP:   net.IP{1, 2, 3, 4},
	Port: 80,
}
fmt.Println(ta.AddrPort())
1.2.3.4:80
1.2.3.4:80

Which is not to say that the problem doesn't exist, of course. I personally would prefer that all functions that are used to convert between net.IP and netip.Addr (and friends) unmap, but I'm not sure how much of a breaking change that would be.

@database64128
Copy link
Contributor

database64128 commented Aug 12, 2022

Re-posting #54234 (comment) since it seems to be more relevant here.

I don't think net.TCPAddr.AddrPort and net.UDPAddr.AddrPort should do the unmapping.

The *net.TCPAddr or *net.UDPAddr instance returned by a LocalAddr() or RemoteAddr() call is directly converted from a system socket address type. The length of the IP slice reflects the address family of the socket. If I open an IPv6 socket, I can expect that the length of the returned IP is always 16. Just like how you can expect the address returned by ReadFromUDPAddrPort and ReadMsgUDPAddrPort on an IPv6 socket is either IPv4-mapped IPv6 or IPv6.

IMO this behavior should be preserved, because this is how the underlying socket works. Those who wish to unmap IPv4-mapped IPv6 addresses can always do the unmapping themselves in their own code.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 12, 2022

If I open an IPv6 socket, I can expect that the length of the returned IP is always 16

And if there's no address family specified, as in net.Listen("udp", ":1234")? I don't think users expect to randomly get either IPv4 or IPv6 addresses depending on which operating system they're on and how that OS was configured. That's not a great API to not know what you're going to get.

@database64128
Copy link
Contributor

database64128 commented Aug 12, 2022

I don't think users expect to randomly get either IPv4 or IPv6 addresses depending on which operating system they're on and how that OS was configured. That's not a great API to not know what you're going to get.

Except it has always been like this. On most systems net.Listen("udp", ":1234") is going to get you a dual-stack IPv6 UDP socket. On OpenBSD and DragonFly BSD this is equivalent to doing net.Listen("udp4", ":1234") on other systems, because these two BSDs do not support dual-stack sockets.

Unless being able to run on OpenBSD and DragonFly BSD is a requirement, one can usually safely assume that net.Listen("udp", ":1234") is going to open a dual-stack IPv6 UDP socket. And in my experience this is usually the way things are being done too.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 12, 2022

I guess the question is whether that's the API we want in Go. Seems like a legacy compatibility wart and we could do better than needlessly exposing mapped addresses to users when we have a type that can distinguish what was really meant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

5 participants