os: File.ReadFrom copy_file_range: bad file descriptor on append only file

[HeikoSchlittermann]

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

$ go version
go version go1.20.4 linux/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="/home/heiko/.cache/go-build"
GOENV="/home/heiko/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/heiko/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/heiko/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/heiko/sdk/go1.20.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/heiko/sdk/go1.20.4/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1360871193=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://go.dev/play/p/c0MHjwdDm4K

Basically io.Copy(os.Stdout, ...)
And calling the program on the (Linux, Bash) commandline:

go run . </etc/hosts >>/tmp/file-test

What did you expect to see?

No error message.

What did you see instead?

2023/05/14 10:32:34 0 write /dev/stdout: copy_file_range: bad file descriptor

Additional Info

This issues seems to be bound to the type of the input. E.g. running the above examples as

cat /etc/hosts | go run . >>/tmp/file-test

it works as expected (despite the useless use of cat).
For me it seems as if Go tries to be a way too smart. Using bytes.NewBufferString("...\n") as input to io.Copy works too.

I'm not sure if this is just my own stupidity, as I didn't find a lot of similar reports.

[seankhliao]

I think this is equivalent to:

package main

import (
	"log"
	"os"
)

func main() {
	f, err := os.Open("in")
	if err != nil {
		log.Fatalln(err)
	}
	out, err := os.OpenFile("out", os.O_APPEND|os.O_CREATE, 0o644)
	if err != nil {
		log.Fatalln(err)
	}
	_, err = out.ReadFrom(f)
	if err != nil {
		log.Fatalln(err)
	}
}

[HeikoSchlittermann]

No, I do not think that this is the same.

• I assume, the flags os.O_* are passed directly to the open(2) syscall. There must be one flag for the requested mode: O_RDONLY, O_WRONLY, O_RDWR, at least according to the manpage of open(2) I find on a recent Debian GNU/Linux.

• W/o passing os.O_WR*, I get write: bad filedescriptor, which is ok.

• W/ passing os.O_WRONLY the append mode works.

Even in case this would reproduce the case I'm describing, I wouldn't compare it. In you example the author of the program can control the flags. In my example I can't. I'm just writing to os.Stdout and I can't control if the caller redirects my output using >, >>, or |.

[bcmills]

(attn @ianlancetaylor; CC @tklauser @panjf2000)

[tklauser]

2023/05/14 10:32:34 0 write /dev/stdout: copy_file_range: bad file descriptor

I think this is because the copy_file_range doesn't support destinations opened with O_APPEND, cf. https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/os/readfrom_linux.go;l=15-19 and https://man7.org/linux/man-pages/man2/copy_file_range.2.html#ERRORS.

That check for f.appendMode in the code linked above should cause the fallback path to be taken instead of copy_file_range. However, we don't seem to set File.appendMode in os.NewFile when creating os.Stdout.

This issues seems to be bound to the type of the input. E.g. running the above examples as

cat /etc/hosts | go run . >>/tmp/file-test

I'm not sure why it doesn't fail in that case. I'd still expect O_APPEND to be set on os.Stdout. However, os.Stdin is a pipe in that case, so maybe we're not hitting the path using copy_file_range?

[gopherbot]

Change https://go.dev/cl/494915 mentions this issue: os: set File.appendMode in newFile if file is opened with O_APPEND

[gopherbot]

Change https://go.dev/cl/494916 mentions this issue: internal/poll, internal/syscall/unix, net: move and export fcntl syscall wrapper

[HeikoSchlittermann]

This issues seems to be bound to the type of the input. E.g. running the above examples as

cat /etc/hosts | go run . >>/tmp/file-test

I'm not sure why it doesn't fail in that case. I'd still expect O_APPEND to be set on os.Stdout. However, os.Stdin is a pipe in that case, so maybe we're not hitting the path using copy_file_range?

Wild guessing w/o reading the (Go) sources: Go tries to be smart if both input and output seem to be regular files. (e.g. >>/dev/null doesn't produce the above error, not sure, if we're not on the copy_file_range path then, or of the OS is forgiving here.)

So, IMHO Go should get even smarter (as suggested by @tklauser) by checking the mode the output is opened as, or less smarter by just avoiding the copy_file_range when operating file handles it doesn't open on its own (0, 1, 2, and maybe even more, if inheritated from a calling process.)

[Masterxilo]

This bug broke my simple program that basically just does "io.Copy(os.Stdout, someData())" whenever that program was used to append to a file: "program >> file.txt" ...

The err you get is: "write /dev/stdout: copy_file_range: bad file descriptor"

[Masterxilo]

was any regression test added to prevent regressions of this in the future?

A basic e2e test like "go run . </etc/hosts >>/tmp/file-test" should definitely be part of the ci build test suite...

[ianlancetaylor]

We believe that this bug is fixed in the upcoming 1.21 release.

If you want to send in a test, that would be welcome. It could be in the os package, or in cmd/go/testdata/script.

[Masterxilo]

Yes I downloaded 1.21rc3 and appending stdout works there. I will see if I find time to contribute.

[gopherbot]

Change https://go.dev/cl/517755 mentions this issue: os: test that copying to append-only files doesn't fail on Linux

[gopherbot]

Change https://go.dev/cl/518255 mentions this issue: os: define TestIssue60181 only on Unix platforms