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.

Sign up for GitHub

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: incorrect sync.Pool usage leads to EINVAL errors on WriteBatch (sendmmsg) #54693

Closed
marten-seemann opened this issue Aug 26, 2022 · 3 comments
Closed

x/net: incorrect sync.Pool usage leads to EINVAL errors on WriteBatch (sendmmsg) #54693

marten-seemann opened this issue Aug 26, 2022 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented Aug 26, 2022 •
edited

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

$ go version
go version go1.19 linux/arm64

with a current version of x/net:

require golang.org/x/net v0.0.0-20220822230855-b0a4917ee28c

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="arm64"
GOBIN=""
GOCACHE="/home/parallels/.cache/go-build"
GOENV="/home/parallels/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/parallels/src/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/parallels/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/parallels/bin/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/parallels/bin/go/pkg/tool/linux_arm64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/parallels/src/go/src/github.com/lucas-clemente/quic-go/go.mod"
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 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1105120780=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I'm adding support for WriteBatch to quic-go. Batching writes of QUIC (UDP) packets leads to a significant speedup of QUIC throughput, due to the reduced number of syscalls. Some connections set OOB, others don't, therefore the OOB field in the ipv{4,6}.Message will sometimes be set, other times it won't.

What did you expect to see?

I expect calls to WriteBatch to succeed.

What did you see instead?

Calls to WriteBatch occasionally return a sendmmsg: invalid argument error. I wrote a minimal example that reproduces the bug (even in the Go Playground): https://go.dev/play/p/lALtpTKEgw5

Why does this happen?

A sync.Pool is used to avoid allocations of the msghdr struct: https://cs.opensource.google/go/x/net/+/b0a4917e:internal/socket/rawconn_mmsg.go;l=39-40.
When the pool returns a msghdr where the Control and Controllen is set (this happens if the previous user of the struct set OOB), it is not properly cleared if no OOB is set: https://cs.opensource.google/go/x/net/+/b0a4917e:internal/socket/msghdr_linux.go;l=14-16.

This leads to Control pointing to a memory location that might have already been reused between the two invocations. The kernel will thus see an invalid mmsghdr passed to the sendmmsg, and (rightfully) return an EINVAL error.

Update: Here's a fix: https://go-review.googlesource.com/c/net/+/425914.
@gopherbot gopherbot added this to the Unreleased milestone Aug 26, 2022
@gopherbot
Copy link

Change https://go.dev/cl/425914 mentions this issue: x/net: properly reset Iov and Control on the msghdr

@gopherbot
Copy link

Change https://go.dev/cl/425915 mentions this issue: x/net: properly reset Iov and Control on the msghdr

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 29, 2022
@heschi
Copy link
Contributor

heschi commented Aug 29, 2022

cc @neild @ianlancetaylor

@marten-seemann marten-seemann mentioned this issue Sep 29, 2022
Open
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
internal/socket: properly reset Iov and Control on the msghdr
ae790d4 
A sync.Pool is used to reduce allocations of the msghdr struct. When using a
struct obtained from the pool, the fields need to be cleared.

Fixes golang/go#54693.

Change-Id: Ie693979abf3aa6bc3c834b9558c5029b376326e6
Reviewed-on: https://go-review.googlesource.com/c/net/+/425915
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@marten-seemann @gopherbot @heschi