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/sys/unix: Undocumented (possibly incorrect?) SendmsgBuffers behavior with empty iovec #56911

Closed
dpifke opened this issue Nov 23, 2022 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dpifke
Copy link
Contributor

dpifke commented Nov 23, 2022

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

$ go version
go version go1.19.3 linux/amd64
$ cat go.sum
golang.org/x/sys v0.2.0 h1:ljd4t30dBnAvMZaQCevtY0xLLD0A+bRZXbgLMLU1F/A=
golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

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="/home/dave/.cache/go-build"
GOENV="/home/dave/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/dave/Source/Go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/dave/Source/Go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.19"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.19/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/dave/Source/keyholder/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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1272319925=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I'm interfacing with the Linux kernel cryptography API using the AF_ALG interface, across two processes. One process is responsible for setting up an AF_ALG (SOCK_SEQPACKET) socket, including setting the key and IV, and then the resulting socket file descriptor is passed to another process (using SCM_RIGHTS), which uses it for encryption or decryption. By design, the first process does not have access to any of the plaintext or ciphertext data, and the second process does not have access to the encryption key.

The bug I was encountering was seemingly an extra '\0' in the decrypted plaintext output and an off-by-one error between plaintext and expected ciphertext length. I discovered this is because an extra byte is silently added by x/sys/unix.sendmsgN() if passed an empty Iovec but non-empty out-of-band data, on a non-SOCK_DGRAM socket:

https://cs.opensource.google/go/x/sys/+/refs/tags/v0.2.0:unix/syscall_linux.go;l=1538;drc=d684c6f886692f8b63049d3dabe4f1911ae979c3;bpv=1;bpt=1

As far as I can tell, this behavior is undocumented. I believe some socket types require at least one byte of data with OOB, but AF_ALG sockets do not. (The Linux kernel documentation references https://www.chronox.de/libkcapi.html as the canonical client implementation, and it follows the pattern of OOB-but-empty-data to set up the socket, and then it uses vmsplice() to send the non-OOB data.)

The following call results in the extra byte being sent to the socket (error handling and populating var oob []byte omitted):

unix.SendmsgBuffers(fd, nil, oob, nil, unix.MSG_MORE)

This code works as expected:

msg := &unix.Msghdr{
    Control: &oob[0],
    // Remaining fields, including Iov and Iovlen are zero
}
msg.SetControllen(len(oob))
unix.Syscall(unix.SYS_SENDMSG, uintptr(fd), uintptr(unsafe.Pointer(msg)), unix.MSG_MORE)

What did you expect to see?

An empty [][]byte resulting in no data being sent.

The documentation for SendmsgBuffers states, "the function returns the number of bytes written to the socket."

What did you see instead?

A '\0' byte being sent to the socket unexpectedly.

SendmsgBuffers returns 0, but actually sends 1 byte to the socket.

Possible solutions/topics for discussion

The implicit extra byte with Sendmsg appears to date back to the very first Go commit, so it probably comes from Plan 9. SendmsgBuffers is, however, relatively new (#52885) so maybe its API/behavior is not as set in stone. Whether or not their behavior in this regard should diverge is probably a topic for debate.

Should SOCK_SEQPACKET be treated the same as SOCK_DGRAM, and excluded from having the extra byte sent? If so, should the socket types (maybe just SOCK_STREAM) that require special empty-iovec handling be an allowlist instead of a denylist?

Should the SendmsgBuffers return value be updated to reflect the actual number of bytes sent when passed an empty [][]byte?

Instead of silently adding an invisible byte, should an error be returned if SendmsgBuffers is not supposed to be called with an empty [][]byte?

Even if no changes to behavior are warranted, should SendmsgBuffer's documentation be improved? (Yes, I am volunteering to do so. :))

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 23, 2022
@gopherbot gopherbot added this to the Unreleased milestone Nov 23, 2022
@dpifke
Copy link
Contributor Author

dpifke commented Nov 23, 2022

Tangentially related: #32465

@ianlancetaylor
Copy link
Contributor

SendmsgBuffers and RecvmsgBuffers should do the right thing if you pass consistent sets of buffer arguments. Of course not all applications work that way.

More documentation is probably a good idea.

On Linux we already check for SOCK_DGRAM in deciding whether to use the extra byte. Can we check for AF_ALG?

This issue doesn't come from Plan 9 as far as I know, it's because of how TCP works. TCP doesn't actually have out-of-band data. What it has is an urgent pointer. The convention for sending out-of-band data on TCP means sending the data with the urgent pointer pointing to the byte after the out-of-band data. So you need to make sure that there is a byte there for the urgent pointer to point to. In C you have to be aware of this. Go tries to take care of it for you. But if you aren't consistent with calls to Sendmsg, etc., then that extra byte does show up.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 23, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Dec 7, 2022

In triage, @ianlancetaylor notes that it would be pretty hard to change the behavior now, but the documentation could be better.

@gopherbot
Copy link

Change https://go.dev/cl/456816 mentions this issue: unix: improve documentation for Sendmsg functions

@golang golang locked and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants