x/sys/windows Crash in ReadFile() when race enabled

[derekwbrown]

Go version

go 1.21.5

Output of go env in your module/workspace:

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\db\AppData\Local\go-build
set GOENV=C:\Users\db\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=c:\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=c:\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go\1.21.5\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=c:\go\1.21.5\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.21.5
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\src\datadog-windows-procmon\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 -ffile-prefix-map=C:\Users\db\AppData\Local\Temp\go-build3031818741=/tmp/go-build -gno-record-gcc-switches

What did you do?

I have windows/go code which crashes unexpectedly.
Specifically, for ReadFile() and WriteFile() any call crashes when the third parameter done is Nil, which is a legal value, when raceenabled

func ReadFile(fd Handle, p []byte, done *uint32, overlapped *Overlapped) error {
	err := readFile(fd, p, done, overlapped)
	if raceenabled {
		if *done > 0 {
			raceWriteRange(unsafe.Pointer(&p[0]), int(*done))
		}
		raceAcquire(unsafe.Pointer(&ioSync))
	}
	return err
}

What did you see happen?

Code crashes when race is enabled and done is nil.

What did you expect to see?

I would expect it to not crash.
the done pointer may be nil when initiating an overlapped operation; in this case the done value isn't filled in until the operation completes (by calling GetQueuedCompletionStatus().

[qmuntal]

Thanks for reporting this bug @derekwbrown. ReadFile does define done as optional, so we should also support that.

[gopherbot]

Change https://go.dev/cl/559375 mentions this issue: windows: support nil done parameter in ReadFile and WriteFile

[derekwbrown]

@qmuntal the fix provided will fix the crash. Howver, it will break code that uses ReadFile() in this way. From the ReadFile() docs:

. Use NULL for this parameter if this is an asynchronous operation to avoid potentially erroneous results.

However, the code change is to simply pass lpNumberOfBytesRead as a pointer to a local variable.

[gopherbot]

Change https://go.dev/cl/559502 mentions this issue: Revert "windows: support nil done parameter in ReadFile and WriteFile"

[qmuntal]

@qmuntal the fix provided will fix the crash. Howver, it will break code that uses ReadFile() in this way. From the ReadFile() docs:
. Use NULL for this parameter if this is an asynchronous operation to avoid potentially erroneous results.
However, the code change is to simply pass lpNumberOfBytesRead as a pointer to a local variable.

Thanks for catching this. Although it is true that the docs warns about passing a non-null lpNumberOfBytesRead for overlapped operations, it does not say that it can't be done, and CL 559375 is safe as per https://devblogs.microsoft.com/oldnewthing/20230215-00/?p=107832:

So it’s okay to pass a non-null lpNumberOfBytesRead to Read­File, even when issuing asynchronous I/O, provided that you do so into a different variable from the one that the completion routine uses.

What I want to avoid is users side stepping the race detector by just passing a nil done. A better solution that is more "safe" as per the docs could be this:

func ReadFile(fd Handle, p []byte, done *uint32, overlapped *Overlapped) error {
	var n uint32
        if done == nil && overlapped == nil {
                // Non-asynchronous case, done must be set.
                done = &n
        }
	err := readFile(fd, p, &n, overlapped)
	if raceenabled {
		if done != nil && *done > 0 {
			raceWriteRange(unsafe.Pointer(&p[0]), int(n))
		}
		raceAcquire(unsafe.Pointer(&ioSync))
	}
	return err
}

[derekwbrown]

so this case

 if done == nil && overlapped == nil {

is explicitly an error:

This parameter can be NULL only when the lpOverlapped parameter is not NULL.

So, unless the caller is doing something blatantly wrong, then the assignment done = &n will never be called.
(and if the caller does this, then readFile will return with an error and no read will actually occur)

So, if overlapped is not nil, then it's OK (and in fact expected) that done is nil, and can/should proceed with calling readFile with no lplpNumberOfBytesRead param.

if overlapped is nil, then done must not be nil, and so everything's fine.

Given that, I think your suggestion above simplifies back to

func ReadFile(fd Handle, p []byte, done *uint32, overlapped *Overlapped) error {
   err := readFile(fd, p, done, overlapped)
	if raceenabled {
   	if done != nil && *done > 0 {
   		raceWriteRange(unsafe.Pointer(&p[0]), int(*done))
   	}
   	raceAcquire(unsafe.Pointer(&ioSync))
      }
	return err
}

(or similar; it's not clear to me whether the raceAcquire() needs to be called )

Full disclosure... I don't totally understand what the race detector is testing/how it's designed to work. But in general, overlapped I/O requires care (by the caller). If the caller initiates overlapped I/O, the caller guarantees the buffer to be valid and not used until the operation completes, via separate call to GetOverlappedResult() or GetQueuedCompletionStatus() So trying to do race detection on the buffer in the overlapped case doesn't seem valid, at least to my understanding.

[derekwbrown]

@qmuntal any update?