golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.38k stars 17.59k forks source link

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

Closed kasparjarek closed 1 year ago

kasparjarek commented 2 years ago

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 https://github.com/golang/go/pull/53555 . What do you think?

gopherbot commented 2 years ago

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

ZekeLu commented 2 years ago

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

https://github.com/golang/go/blob/416c953960a475b7418b5c6aef0f46dd102b9129/src/net/cgo_unix.go#L341-L348

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 commented 2 years ago

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 commented 2 years ago

CC @bradfitz @josharian

ZekeLu commented 2 years ago

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

gopherbot commented 2 years ago

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

zhlsunshine commented 1 year ago

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?