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: LookupNetIP returns IPv4 in IPv6 instead of IPv4 #53554

Closed
kasparjarek opened this issue Jun 26, 2022 · 7 comments
Closed

net: LookupNetIP returns IPv4 in IPv6 instead of IPv4 #53554

kasparjarek opened this issue Jun 26, 2022 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kasparjarek
Copy link

kasparjarek commented Jun 26, 2022

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

$ go version
go version go1.18.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jaroslav.kaspar/Library/Caches/go-build"
GOENV="/Users/jaroslav.kaspar/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/jaroslav.kaspar/go/pkg/mod"
GONOPROXY="github.com/wandera/*"
GONOSUMDB="github.com/wandera/*"
GOOS="darwin"
GOPATH="/Users/jaroslav.kaspar/go"
GOPRIVATE="github.com/wandera/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.18.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.18.2/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.2"
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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/k5/bmlsxyfn1tjd50_r0x637flc0000gp/T/go-build3646431415=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"context"
	"fmt"
	"net"
)

func main() {
	ips, err := net.DefaultResolver.LookupNetIP(context.Background(), "ip4", "google.com")
	if err != nil {
		panic(err)
	}

	fmt.Printf("%+v\n", ips)
	fmt.Printf("is IPv4: %t\n", ips[0].Is4())
	fmt.Printf("is IPv6: %t\n", ips[0].Is6())
	fmt.Printf("is IPv4 in IPv6: %t\n", ips[0].Is4In6())
}

What did you expect to see?

[142.251.36.142]
is IPv4: true
is IPv6: false
is IPv4 in IPv6: false

What did you see instead?

[::ffff:142.251.36.142]
is IPv4: false
is IPv6: true
is IPv4 in IPv6: true

The method LookupNetIP is currently just a wrapper around the old way. But the old way represents IPv4 in 16 bytes slice, so when this wrapper parses the slice it will always create an IPv6 address. For IPv4 addresses, it's IPv4 in IPv6. It makes it harder to manipulate with a new address representation. For example I cannot just simply compare result of netip.MustParseAddr("142.251.36.142") with the same address returned by LookupNetIP(). The comparison results in non-zero value.

I would propose converting the old address into 4 bytes slice (if it's IPv4 address) before parsing #53555 . What do you think?

kasparjarek added a commit to kasparjarek/go that referenced this issue Jun 26, 2022
The method is currently just a wrapper around the old way. But the old way
represents IPv4 in 16 bytes slice, so when this wrapper parses the slice it
will always create IPv6 address. For IPv4 addreses it's IPv4 in IPv6.
The fix just converts the old IPv4 address representation to 4 bytes slice
before parsing, so it's parsed as IPv4 and not IPv4 in IPv6.

Fixes golang#53554
@gopherbot
Copy link

Change https://go.dev/cl/414275 mentions this issue: net: fix LookupNetIP to return IPv4 addresses instead of IPv4 in IPv6

@ZekeLu
Copy link
Contributor

ZekeLu commented Jun 26, 2022

It seems that the original IP is converted to IPv6 here:

go/src/net/cgo_unix.go

Lines 341 to 348 in 416c953

func copyIP(x IP) IP {
if len(x) < 16 {
return x.To16()
}
y := make(IP, len(x))
copy(y, x)
return y
}

Maybe it's better to remove the conversion?

  func copyIP(x IP) IP {
- 	if len(x) < 16 {
- 		return x.To16()
-	}
  	y := make(IP, len(x))
  	copy(y, x)
  	return y
  }

I run ./all.bash after this change and all tests passed (on Linux). But I'm not sure whether this func is covered by the tests.

BTW, the 3 lines were added as part of the commit to fix #6465. I have tested on tip with the 3 lines removed and can not reproduce #6465.

@kasparjarek
Copy link
Author

kasparjarek commented Jun 26, 2022

You are right. It seems quite weird to me that the copyIP function converts it to 16 bytes. If it's not actually necessary I can change the pull request to rather remove the three lines. I guess I will wait for reviewers to know what they think about it.

@ianlancetaylor
Copy link
Contributor

CC @bradfitz @josharian

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 28, 2022
@ZekeLu
Copy link
Contributor

ZekeLu commented Jul 1, 2022

@kasparjarek I'm sorry that my suggested approach turns out to need more changes than that. So I will send a CL myself.

@gopherbot
Copy link

Change https://go.dev/cl/415580 mentions this issue: net: store IPv4 returned from cgo resolver as 4-byte slice net.IP

@zhlsunshine
Copy link

zhlsunshine commented Oct 25, 2022

Hi @kasparjarek, how about the implementation in PR#56406? Let's just change the calling from netip.AddrFromSlice to netip.ParseAddr which already do the IPv4/6 separation in LookupNetIP. Does it make sense?

romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
net.IP states that a 16-byte slice can still be an IPv4 address.
But after netip.Addr is introduced, it requires extra care to keep
it as an IPv4 address when converting it to a netip.Addr using
netip.AddrFromSlice.

To address this issue, let's change the cgo resolver to return
4-byte net.IP for IPv4. The change will save us 12 bytes too.

Please note that the go resolver already return IPv4 as 4-byte
slice.

The test TestResolverLookupIP has been modified to cover this
behavior. So no new test is added.

Fixes golang#53554.

Change-Id: I0dc2a59ad785c0c67a7bc22433105529f055997f
GitHub-Last-Rev: bd7bb2f
GitHub-Pull-Request: golang#53638
Reviewed-on: https://go-review.googlesource.com/c/go/+/415580
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
6 participants