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

Open
kasparjarek opened this issue Jun 26, 2022 · 6 comments · May be fixed by #53555 or #53638
Open

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

kasparjarek opened this issue Jun 26, 2022 · 6 comments · May be fixed by #53555 or #53638
Labels
NeedsInvestigation

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

gopherbot commented Jun 26, 2022

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

@ZekeLu
Copy link

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

ianlancetaylor commented Jun 26, 2022

CC @bradfitz @josharian

@seankhliao seankhliao added the NeedsInvestigation label Jun 28, 2022
@ZekeLu
Copy link

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

gopherbot commented Jul 1, 2022

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

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