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

Jump to bottom

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
Closed

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

marten-seemann opened this issue Nov 8, 2020 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@marten-seemann
Copy link
Contributor

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

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
}

@networkimprov
Copy link

cc @odeke-em @ianlancetaylor

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 13, 2020
@cagedmantis
Copy link
Contributor

/cc @bradfitz @ianlancetaylor

@marten-seemann marten-seemann mentioned this issue Nov 16, 2020
Closed
@marten-seemann
Copy link
Contributor Author

Suggested fix: https://go-review.googlesource.com/c/net/+/271927.

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@cagedmantis @networkimprov @marten-seemann @gopherbot