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

os: possible regression from Go 1.23 to Go 1.24 when opening DevNull with O_TRUNC #71752

Closed
mvdan opened this issue Feb 14, 2025 · 17 comments
Closed
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Feb 14, 2025

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

$ go version
go version go1.24.0 linux/amd64

Does this issue reproduce with the latest release?

Yes; and it also reproduces on tip.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/mvdan/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/mvdan/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3169099861=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/home/mvdan/go/pkg/mod'
GONOPROXY='github.com/cue-unity'
GONOSUMDB='github.com/cue-unity'
GOOS='linux'
GOPATH='/home/mvdan/go'
GOPRIVATE='github.com/cue-unity'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/mvdan/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'
uname -sr: Linux 6.13.2-arch1-1
LSB Version:	n/a
Distributor ID:	Arch
Description:	Arch Linux
Release:	rolling
Codename:	n/a
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.41.
gdb --version: GNU gdb (GDB) 16.2

What did you do?

Take the following program:

$ cat f.go
package main

import (
	"fmt"
	"os"
)

func main() {
	f, err := os.OpenFile(os.DevNull, os.O_TRUNC, 0)
	fmt.Println(f != nil, err)
}

Then I run it on the latest Go 1.23 and 1.24, on both Linux (the host) and Windows (via wine):

$ for goos in linux windows; do for go in go1.23.6 go1.24.0; do echo $goos $go; GOOS=$goos GOTOOLCHAIN=$go go run f.go; done; done
linux go1.23.6
true <nil>
linux go1.24.0
true <nil>
windows go1.23.6
true <nil>
windows go1.24.0
false open NUL: Invalid handle.

What did you expect to see?

Consistent behavior, or otherwise, some sort of changelog note in https://go.dev/doc/go1.24 to explain what happened and why.

What did you see instead?

It appears that opening os.DevNull, aka NUL on Windows, with O_TRUNC is now deemed an invalid operation, when before it wasn't.

It's not clear to me whether the new behavior on Windows in terms of this edge case is correct or not, but it certainly caught me by surprise. Not only because it changed from Go 1.23, but also because on Linux the operation still works OK.

I am using Wine here as a quick way to test this, but this error happens on real Windows too; see https://github.com/mvdan/sh/actions/runs/13274870177, where go test succeeded on Go 1.23 on Windows, but failed on Go 1.24 with the same "Incorrect function" errors.

I suspect one of @qmuntal's changes, perhaps https://go-review.googlesource.com/c/go/+/618835 or https://go-review.googlesource.com/c/go/+/620576, could have caused this?

My apologies for not having spotted this issue earlier; I test Go at master continuosuly, but only on Linux, as Wine only gets me so far.

@mvdan mvdan added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Feb 14, 2025
@mvdan
Copy link
Member Author

mvdan commented Feb 14, 2025

I should clarify that not using O_TRUNC does resolve this regression. So I could work around this regression via a check in my code like...

if path == os.DevNull {
	flag &^= os.O_TRUNC
}

but that possibly just papers over an unintentional regression that should be fixed regardless.

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 14, 2025
@ianlancetaylor
Copy link
Member

This is most likely due to https://go.dev/cl/618836 for #38225.

It's weird to pass O_TRUNC without also passing O_RDWR or O_WRONLY. On Unix in that case the O_TRUNC flag is simply ignored. Perhaps we should do the same on Windows.

@ianlancetaylor ianlancetaylor added this to the Go1.25 milestone Feb 14, 2025
@mvdan
Copy link
Member Author

mvdan commented Feb 14, 2025

To be clear, the flags I was passing in the real program were os.O_WRONLY | os.O_CREATE | os.O_TRUNC. The program above was my attempt at simplifying the reproducer to the point that I could tell which flag was causing breakage. But, for the sake of not showing a nonsensical example, here is the more realistic one:

$ cat f.go
package main

import (
	"fmt"
	"os"
)

func main() {
	f, err := os.OpenFile(os.DevNull, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0)
	fmt.Println(f != nil, err)
}
$ for goos in linux windows; do for go in go1.23.6 go1.24.0; do echo $goos $go; GOOS=$goos GOTOOLCHAIN=$go go run f.go; done; done
linux go1.23.6
true <nil>
linux go1.24.0
true <nil>
windows go1.23.6
true <nil>
windows go1.24.0
false open NUL: Invalid handle.

Should this be milestoned for a backport fix to 1.24?

@ianlancetaylor
Copy link
Member

Thanks for the O_WRONLY note. That's too bad. I was hoping this would be trivial.

We should probably backport but let's give @qmuntal a chance to take a look. It may be that we have to special case NUL, in which case we need to think about where the special case should go: os.Open or syscall.Open?

@mvdan
Copy link
Member Author

mvdan commented Feb 14, 2025

Sounds good. There's no rush from my side, as thankfully I can work around the bug for now.

@qmuntal
Copy link
Member

qmuntal commented Feb 17, 2025

Weird, I can't reproduce this regression locally on a real Windows machine. Go 1.23 didn't support truncating os.DevNull, nor does Go 1.24.

>type main.go 
package main

import (
        "fmt"
        "os"
)

func main() {
        f, err := os.OpenFile(os.DevNull, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0)
        fmt.Println(f != nil, err)
}

>go1.23.6 run main.go 
false open NUL: Incorrect function.

>go1.24.0 run main.go
false open NUL: Incorrect function.

Could it be an issue with Wine? If that's the case, then IMO this issue belongs to Wine, not to Go. Another option would be to special case os.DevNull to make it consistent with the Linux behavior (what about other OSes?). I'm also good with that, although corner cases will be tricky to handle (e.g. NULL.txt is also a path to a null device on some Windows versions).

@mvdan
Copy link
Member Author

mvdan commented Feb 17, 2025

I don't think this issue relates to Wine; I originally uncovered it on GitHub Actions via their windows-2022 runner, which as far as I can tell runs real Windows.

See https://github.com/mvdan/sh/actions/runs/13274870177/job/37062363963, succeeding on Go 1.23.5, and https://github.com/mvdan/sh/actions/runs/13274870177/job/37062364367, failing on Go 1.24.0.

Perhaps the way I reduced the failing test made it only show the regression on Wine, but not on real Windows? If so, try running go test ./interp on that repository at commit mvdan/sh@df624fe.

My only other guess is that, somehow, this regression affects Wine and the version of Windows used by GitHub Actions, but not the one you have installed.

@qmuntal
Copy link
Member

qmuntal commented Feb 18, 2025

Perhaps the way I reduced the failing test made it only show the regression on Wine, but not on real Windows? If so, try running go test ./interp on that repository at commit mvdan/sh@df624fe.

Thanks for the pointer, I can reproduce it now. Setting the S_IWRITE permission bit in the os.OpenFile call made the trick:

package main

import (
	"fmt"
	"os"
)

func main() {
	f, err := os.OpenFile(os.DevNull, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
	fmt.Println(f != nil, err)
}

Worth noting that the function fails if os.O_CREATE is not set, as in such case Windows does the same as we are doing in Go 1.24: truncate the file using SetFileInformationByHandle, which returns ERROR_INVALID_FUNCTION when the handle is the NUL device.

Will investigate and report back.

@mvdan
Copy link
Member Author

mvdan commented Feb 18, 2025

Thanks! FWIW I don't oppose a conclusion like "never use O_CREATE or O_TRUNC on DevNull as it may fail on some platforms". This is just not something that is documented or warned about right now.

@qmuntal
Copy link
Member

qmuntal commented Feb 18, 2025

I've submitted CL 650276, which makes syscall.OpenFile behave as in Go 1.23 without losing the improvements added in CL 618836. The changes are small to facilitate backporting. @mvdan can you check it also makes Wine happy?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/650276 mentions this issue: syscall: don't truncate newly created files on Windows

@mvdan
Copy link
Member Author

mvdan commented Feb 18, 2025

Thanks for the fix! That's the sort of fix I imagined might be necessary.

Just so I understand, NUL on Windows with O_CREATE|O_TRUNC shouldn't be creating a new file on disk, right? I'm slightly confused by the commit message saying "don't truncate newly created files".

I assume your reproducer above, as well as the added test in the CL, now pass on your Windows box. Interestingly, they still seem to fail on wine for me, if I cherry pick your commit on master:

$ go test -run TestOpenFileDevNull
PASS
ok  	os	0.001s
$ wine --version
wine-10.1
$ go version
go version devel go1.25-0ecb3c41b4 2025-02-18 18:24:15 +0000 linux/amd64
$ GOOS=windows go test -run TestOpenFileDevNull
--- FAIL: TestOpenFileDevNull (0.00s)
    os_test.go:3854: OpenFile(DevNull): open NUL: Invalid handle.
FAIL
exit status 1
FAIL	os	0.316s

Lots of other tests fail for me with Wine via GOOS=windows go test -short os, so I'm not sure to what extent that is a problem. I was only using Wine for the sake of debugging the issue, but I mainly care about real Windows here.

@qmuntal
Copy link
Member

qmuntal commented Feb 19, 2025

Just so I understand, NUL on Windows with O_CREATE|O_TRUNC shouldn't be creating a new file on disk, right? I'm slightly confused by the commit message saying "don't truncate newly created files".

Yep, it doesn't create any file. It is a bit confusing because the NUL device (as other Windows devices) always exist, but behave as if they were brand new files (they don't have state) when opening them regardless of the creation mode you use.

@qmuntal
Copy link
Member

qmuntal commented Feb 19, 2025

@gopherbot please backport this to go1.24 release.

This issue causes a regression when opening os.DevNull with the os.O_TRUNC bit set.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #71836 (for 1.24).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 19, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/650597 mentions this issue: [release-branch.go1.24] syscall: don't truncate newly created files on Windows

gopherbot pushed a commit that referenced this issue Feb 19, 2025
…n Windows

There is no need for syscall.OpenFile to truncate newly created files.
Some special Windows files, like the NUL device, can't be
truncated, so we should avoid truncating unless it is really necessary.

For #71752
Fixes #71836

Change-Id: I8238048594f706f6a5281053d55cfe3dc898828d
Reviewed-on: https://go-review.googlesource.com/c/go/+/650276
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
(cherry picked from commit 4267fd3)
Reviewed-on: https://go-review.googlesource.com/c/go/+/650597
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Commit-Queue: Ian Lance Taylor <[email protected]>
Reviewed-by: Quim Muntal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Development

No branches or pull requests

6 participants