os: syscall.FindNextFile Crash Regression in Go 1.20 / Windows

[klauspost]

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

Bad:
λ go version
go version go1.20.3 windows/amd64

Good:
λ go version
go version go1.19.5 windows/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
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=e:\temp\gocache
set GOENV=C:\Users\klaus\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=e:\gopath\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=e:\gopath
set GOPRIVATE=
set GOPROXY=https://goproxy.io,https://goproxy.dev,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.3
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=e:\temp\dir-repro\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=e:\temp\wintemp\go-build712183971=/tmp/go-build -gno-record-gcc-switches

What did you do?

go run main.go on https://go.dev/play/p/YVSA2Lyoxdq

Above is a simplified version of minio read dir code

The f.Seek behavior has also changed on directories, but I can live with that.

What did you expect to see?

λ go run main.go
:)

Output in Go 1.19.x

What did you see instead?

Exception 0xc0000005 0x1 0x1ac 0x7ffcf4a5168d
PC=0x7ffcf4a5168d

runtime.cgocall(0x78ff80, 0x88f1a0)
	C:/go/src/runtime/cgocall.go:157 +0x4a fp=0xc0000c78e8 sp=0xc0000c78b0 pc=0x7338ca
syscall.SyscallN(0x7ffcf41308a0?, {0xc0000c7980?, 0x3?, 0x0?})
	C:/go/src/runtime/syscall_windows.go:557 +0x109 fp=0xc0000c7960 sp=0xc0000c78e8 pc=0x78b449
syscall.Syscall(0xc000022b10?, 0xc000028000?, 0x1bd03eeba18?, 0x70?, 0x1?)
	C:/go/src/runtime/syscall_windows.go:495 +0x3b fp=0xc0000c79a8 sp=0xc0000c7960 pc=0x78b21b
syscall.findNextFile1(0xc0000c7ab0?, 0x743090?)
	C:/go/src/syscall/zsyscall_windows.go:625 +0x6c fp=0xc0000c79f8 sp=0xc0000c79a8 pc=0x7ac6ac
syscall.FindNextFile(0xc0000181a0?, 0xc0000c7ca4)
	C:/go/src/syscall/syscall_windows.go:1144 +0x5d fp=0xc0000c7c68 sp=0xc0000c79f8 pc=0x7aba7d
main.readDirWithOpts({0xc0000181a0, 0x1f})
	E:/temp/dir-repro/main.go:46 +0x1f5 fp=0xc0000c7f38 sp=0xc0000c7c68 pc=0x7cc9d5
main.main()
	E:/temp/dir-repro/main.go:15 +0x3b fp=0xc0000c7f80 sp=0xc0000c7f38 pc=0x7cc75b
runtime.main()
	C:/go/src/runtime/proc.go:250 +0x1f7 fp=0xc0000c7fe0 sp=0xc0000c7f80 pc=0x7662b7
runtime.goexit()
	C:/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc0000c7fe8 sp=0xc0000c7fe0 pc=0x78e661

goroutine 2 [force gc (idle)]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	C:/go/src/runtime/proc.go:381 +0xd6 fp=0xc000081fb0 sp=0xc000081f90 pc=0x7666d6
runtime.goparkunlock(...)
	C:/go/src/runtime/proc.go:387
runtime.forcegchelper()
	C:/go/src/runtime/proc.go:305 +0xb2 fp=0xc000081fe0 sp=0xc000081fb0 pc=0x7664f2
runtime.goexit()
	C:/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc000081fe8 sp=0xc000081fe0 pc=0x78e661
created by runtime.init.6
	C:/go/src/runtime/proc.go:293 +0x25

goroutine 3 [GC sweep wait]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	C:/go/src/runtime/proc.go:381 +0xd6 fp=0xc000083f80 sp=0xc000083f60 pc=0x7666d6
runtime.goparkunlock(...)
	C:/go/src/runtime/proc.go:387
runtime.bgsweep(0x0?)
	C:/go/src/runtime/mgcsweep.go:278 +0x8e fp=0xc000083fc8 sp=0xc000083f80 pc=0x75142e
runtime.gcenable.func1()
	C:/go/src/runtime/mgc.go:178 +0x26 fp=0xc000083fe0 sp=0xc000083fc8 pc=0x746866
runtime.goexit()
	C:/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc000083fe8 sp=0xc000083fe0 pc=0x78e661
created by runtime.gcenable
	C:/go/src/runtime/mgc.go:178 +0x6b

goroutine 4 [GC scavenge wait]:
runtime.gopark(0xc00008a000?, 0x80a9e8?, 0x1?, 0x0?, 0x0?)
	C:/go/src/runtime/proc.go:381 +0xd6 fp=0xc000091f70 sp=0xc000091f50 pc=0x7666d6
runtime.goparkunlock(...)
	C:/go/src/runtime/proc.go:387
runtime.(*scavengerState).park(0x88eaa0)
	C:/go/src/runtime/mgcscavenge.go:400 +0x53 fp=0xc000091fa0 sp=0xc000091f70 pc=0x74f353
runtime.bgscavenge(0x0?)
	C:/go/src/runtime/mgcscavenge.go:628 +0x45 fp=0xc000091fc8 sp=0xc000091fa0 pc=0x74f945
runtime.gcenable.func2()
	C:/go/src/runtime/mgc.go:179 +0x26 fp=0xc000091fe0 sp=0xc000091fc8 pc=0x746806
runtime.goexit()
	C:/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc000091fe8 sp=0xc000091fe0 pc=0x78e661
created by runtime.gcenable
	C:/go/src/runtime/mgc.go:179 +0xaa

goroutine 5 [finalizer wait]:
runtime.gopark(0x766a52?, 0x877780?, 0x0?, 0x0?, 0xc000085f70?)
	C:/go/src/runtime/proc.go:381 +0xd6 fp=0xc000085e28 sp=0xc000085e08 pc=0x7666d6
runtime.runfinq()
	C:/go/src/runtime/mfinal.go:193 +0x107 fp=0xc000085fe0 sp=0xc000085e28 pc=0x7458c7
runtime.goexit()
	C:/go/src/runtime/asm_amd64.s:1598 +0x1 fp=0xc000085fe8 sp=0xc000085fe0 pc=0x78e661
created by runtime.createfing
	C:/go/src/runtime/mfinal.go:163 +0x45
rax     0x5130092000
rbx     0x88f1a0
rcx     0x1a4
rdi     0x17c
rsi     0xc0000c7920
rbp     0xc0000c78d8
rsp     0x51303ff5d0
r8      0x0
r9      0x0
r10     0x333
r11     0x60000442
r12     0xc0000c7a08
r13     0x0
r14     0xc00007c000
r15     0x1
rip     0x7ffcf4a5168d
rflags  0x10202
cs      0x33
fs      0x53
gs      0x2b

Process finished with the exit code 2

Go 1.20.3 output.

This seems to be caused by changes in os package and not syscall. Therefore the title.

A guess be that this is caused by #52747

Edit: Verified this also reproduces in 1.20.3

[klauspost]

@qmuntal

[qmuntal]

This is unfortunate. The problem is that you are passing the file handle -retrieved via os#File.Fd- to syscall.FindNextFile, which expects a handle initialized via syscall.FindFirstFile, but we don't use that type of handles for directories since go1.20.

Assuming that os#File.Fd returns a handle that can be directly used with syscall.FindNextFile is leaky and relies on an undocumented behavior. The correct approach is to construct the handle using syscall.FindFirstFile, like in here.

Additionally, I don't see any other reasonable approach to fix #52747 nor #36019.

Having said this, I would defer to @rsc and @rolandshoemaker the decision on what to do here. In case we decide that this should be reverted, then CL 452995 would have to be reverted to.

[qmuntal]

@golang/windows

[harshavardhana]

This is unfortunate. The problem is that you are passing the file handle -retrieved via os#File.Fd- to syscall.FindNextFile, which expects a handle initialized via syscall.FindFirstFile, but we don't use that type of handles for directories since go1.20.

Assuming that os#File.Fd returns a handle that can be directly used with syscall.FindNextFile is leaky and relies on an undocumented behavior. The correct approach is to construct the handle using syscall.FindFirstFile, like in here.

We could definitely do this change, as it seems harmless.

[bcmills]

Agreed: if you want to use syscall functions that require a file handle constructed in a specific way, then you should use syscall to construct exactly the sort of handle you need.

Mixing and matching os and syscall works for a few cases on Unix platforms (where file descriptors are more uniform), but seems too fragile on Windows.

[klauspost]

Worked around the both regressions upstream.

[bcmills]

Given that we have only one report of a broken package so far and it was straightforward to fix, I think we should leave the os.File behavior as is. (If we get reports of more widespread breakage we can revisit that.)