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

x/net: ReadBatch fails if conn passed to ipv4.New{Packet}Conn is not a net.{TCP/UDP}Conn #42444

Closed
marten-seemann opened this issue Nov 8, 2020 · 5 comments

Comments

@marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Nov 8, 2020

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

$ go version
go version go1.15.3 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="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/marten/Library/Caches/go-build"
GOENV="/Users/marten/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/marten/src/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/marten/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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=/var/folders/q0/b5ynf00142l7bl9sp8y098zr0000gn/T/go-build393900320=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

When passing a wrapped connection to ipv4.NewPacketConn(), calls to ReadBatch() will fail with an invalid connection error:

type wrappedConn { *net.UDPConn }
conn := ipv4.NewPacketConn(&wrappedConn{...})
conn.ReadBatch() // will return the "invalid connection" error

This is unexpected, as NewPacketConn accepts a net.PacketConn, and does not require a net.UDPConn.
Analogously for NewConn with a net.TCPConn.

The problem here is that the constructor calls socket.NewConn, which does a type assertion: https://github.com/golang/net/blob/ff519b6c91021e6316e1df005bc19f266994ddda/internal/socket/rawconn.go#L21-L41, which will fail if the UPDConn is wrapped.

What did you expect to see?

I expected ReadBatch to work.

What did you see instead?

See above.

@gopherbot gopherbot added this to the Unreleased milestone Nov 8, 2020
@marten-seemann
Copy link
Contributor Author

@marten-seemann marten-seemann commented Nov 8, 2020

While one could argue that ipv4.New{Packet}Conn should take a concrete type (net.TCPConn / net.UDPConn) instead of an interface, there's a solution that doesn't require a breaking API change, while at the same time allowing for wrapped connections to be used: In that case socket.NewConn() would have to do an interface assertion instead of a type assertion.

This is easy to accomplish, since TCPConn, UDPConn as well as IPConn each have methods that are unique to that type. For example, the UDPConn can be identified by its ReadMsgUDP method, the IPConn by its ReadMsgIP and the TCPConn by SetLinger. There are other methods that would also achieve the same result.

socket.NewConn would then look something like this:

func NewConn(c net.Conn) (*Conn, error) {
	var err error
	var cc Conn
	if tcpConn, ok := c.(interface {
		SyscallConn() (syscall.RawConn, error)
		SetLinger() error
	}); ok {
		cc.network = "tcp"
		cc.c, err = tcpConn.SyscallConn()
	} else if udpConn, ok := c.(interface {
		SyscallConn() (syscall.RawConn, error)
		ReadMsgUDP(b, oob []byte) (n, oobn, flags int, addr *net.UDPAddr, err error)
	}); ok {
		cc.network = "udp"
		cc.c, err = udpConn.SyscallConn()
	} else if ipConn, ok := := c.(interface {
		SyscallConn() (syscall.RawConn, error)
		ReadMsgIP(b, oob []byte) (n, oobn, flags int, addr *net.IPAddr, err error)
	}); ok {
		cc.network = "ip"
		cc.c, err = ipConn.SyscallConn()
	} else
		return nil, errors.New("unknown connection type")
	}
	if err != nil {
		return nil, err
	}
	return &cc, nil
}
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Nov 13, 2020

@marten-seemann
Copy link
Contributor Author

@marten-seemann marten-seemann commented Jan 20, 2021

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 2, 2021

Change https://golang.org/cl/271927 mentions this issue: x/net: use interface assertions to determine the net.Conn type in socket.NewConn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants