cmd/compile: in specific conditions in go1.22rc1, Windows SetFileInformationByHandle returns "Invalid access to memory location"

[dagood]

Go version

go1.22rc1

Output of go env in your module/workspace:

set GO111MODULE=off
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\ContainerAdministrator\AppData\Local\go-build
set GOENV=C:\Users\ContainerAdministrator\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\gopath\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\gopath
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=local
set GOTOOLDIR=C:\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.22rc1
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
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=C:\Users\CONTAI~1\AppData\Local\Temp\go-build1889816543=/tmp/go-build -gno-record-gcc-switches

What did you do?

I'm filing this on behalf of moby/moby. This came up in a PR attempting to update from go1.21.5 to go1.22rc1: moby/moby#46982. Unfortunately I haven't been able to get it to repro locally by setting up a simpler repro app, and the moby/moby tests are complex enough that I haven't successfully attempted to run them locally. I forked the project and ran experimental changes in GitHub Actions to try things out, for now.

I wrote more details as I went in microsoft/go#1100, but I'll try to summarize it all here. My GitHub Actions runs are at dagood/moby#1.

What did you see happen?

In GitHub CI, this error shows up reliably for a given commit, on each of the 14 jobs that test it per workflow run:

failed to register layer: re-exec error: exit status 1: output: SetFileInformationByHandle \\?\C:\Windows\SystemTemp\hcs2001018820\Files.$wcidirs$: Invalid access to memory location.

I was able to add some code to print a stack trace upon error, and got this:

runtime/debug.Stack()
	C:/go/src/runtime/debug/stack.go:24 +0x5e
github.com/docker/docker/vendor/github.com/Microsoft/go-winio.SetFileBasicInfo(0xc00007d550, 0xf?)
	C:/gopath/src/github.com/docker/docker/vendor/github.com/Microsoft/go-winio/fileinfo.go:49 +0x85
github.com/docker/docker/vendor/github.com/Microsoft/hcsshim/internal/wclayer.(*legacyLayerWriter).Add(0xc000b02680, {0xc000859410, 0x5}, 0xc0004a1350)
	C:/gopath/src/github.com/docker/docker/vendor/github.com/Microsoft/hcsshim/internal/wclayer/legacy.go:697 +0xb54
github.com/docker/docker/daemon/graphdriver/windows.writeLayerFromTar({0x2b80e[40](https://github.com/dagood/moby/actions/runs/7414601797/job/20176251891#step:12:41), 0xc00007c058}, {0x2bae120, 0xc000097d60}, {0xc00087a[48](https://github.com/dagood/moby/actions/runs/7414601797/job/20176251891#step:12:49)0, 0x80})
	C:/gopath/src/github.com/docker/docker/daemon/graphdriver/windows/windows.go:7[54](https://github.com/dagood/moby/actions/runs/7414601797/job/20176251891#step:12:55) +0x312
github.com/docker/docker/daemon/graphdriver/windows.writeLayer({0x2b80e40, 0xc00007c058}, {0xc00004aa00, 0x3f}, {0xc00004aa80, 0x40}, {0xc0000920b0, 0x1, 0x1})
	C:/gopath/src/github.com/docker/docker/daemon/graphdriver/windows/windows.go:831 +0x2aa
github.com/docker/docker/daemon/graphdriver/windows.writeLayerReexec()
	C:/gopath/src/github.com/docker/docker/daemon/graphdriver/windows/windows.go:792 +0x75
github.com/docker/docker/pkg/reexec.Init(...)
	C:/gopath/src/github.com/docker/docker/pkg/reexec/reexec.go:30
main.main()
	C:/gopath/src/github.com/docker/docker/cmd/dockerd/docker.go:75 +0x196

Because the the error itself mentions SetFileInformationByHandle, it seems like this must be related to 549256: internal/syscall/windows: fix the signature of SetFileInformationByHandle (it involves the same function name and the CL and error are both new in go1.22rc1). However, based on the stack trace, I'm not sure it's linked at all. The CL changes internal/syscall/windows like this:

- //sys	SetFileInformationByHandle(... buf uintptr, bufsize uint32) ...
+ //sys	SetFileInformationByHandle(... buf unsafe.Pointer, bufsize uint32) ...

But fileinfo.go#L38-L55 uses x/sys/windows's implementation, which isn't like either of those, accepting *byte:

//sys	SetFileInformationByHandle(... inBuffer *byte, inBufferLen uint32) ...

func SetFileBasicInfo(f *os.File, bi *FileBasicInfo) error {
	if err := windows.SetFileInformationByHandle(
		windows.Handle(f.Fd()),
		windows.FileBasicInfo,
		(*byte)(unsafe.Pointer(bi)),
		uint32(unsafe.Sizeof(*bi)),
	); err != nil {
		return &os.PathError{Op: "SetFileInformationByHandle", Path: f.Name(), Err: err}
	}
	runtime.KeepAlive(f)
	return nil
}

My next experiment was to try putting %#v of bi in the error message to see if something's special about the values. That could perhaps help me repro this locally with a simpler program. However... adding a fmt.*f causes the error to stop happening!

So, I thought that somehow bi wasn't being kept alive, and that's why trying to format it made it work. I removed the formatting code and added runtime.KeepAlive(bi) after runtime.KeepAlive(f) instead. But the error shows up in this case.

This makes me think the compiler is making some decision differently in go1.22rc1 than it did in go1.21.5, leading to this error when combined with this code.

Maybe it's an issue with the library, but I'm not sure how to properly investigate that, and understanding what changed in go1.22rc1 to make this start showing up will help narrow that down at the very least.

What did you expect to see?

No error.

[dmitshur]

CC @golang/compiler, @golang/windows.

[dagood]

I grabbed the built binaries from a couple GitHub Actions repro/no-repro runs, but dockerd.exe is 31 MB compressed (25 MB max on GitHub), so I'll have to find some storage to host these if that is necessary to reasonably poke at this. They're currently up at failure, success but I don't know what the retention period is.

For now, I ran go tool objdump on the top two funcs: https://gist.github.com/dagood/374b949bf8e9bf636cb9e027f79db698. Not a clean diff, but maybe there's something easy to spot here.

[alexbrainman]

@dagood

Interestingly CreateAcceleratorTableW also reports ERROR_NOACCESS in #64729 . :-)

Perhaps it is something related to garbage collector.

Alex

[qmuntal]

Thanks for the pointer @alexbrainman. The issue #64729 has the same failure mode as this one, and an easier reproducer. I'm pretty sure that the root cause in both cases is the same as in CL 549256): one of the parameter's memory is not correctly aligned. In fact, ERROR_NOACCESS can be returned when the kernel detects a STATUS_DATATYPE_MISALIGNMENT error.

For this particular issue, the problem is in the winio.FileBasicInfo definition:

type FileBasicInfo struct {
	CreationTime, LastAccessTime, LastWriteTime, ChangeTime windows.Filetime
	FileAttributes                                          uint32
	_                                                       uint32 // padding
}

The Go compiler uses an alignment of 4 bytes, as windows.Filetime is a struct containing two uint32 values. On the other hand, the C compiler uses an alignment of 8 bytes, as FileBasicInfo defines the time properties using LARGE_INTEGER, which is a uint64.

This is not the first time that windows.Filetime causes alignment issues, and it's a difficult generic fix unless #19057 is approved.

The solution here, if I'm not wrong, is to apply the same fix as in https://go-review.googlesource.com/c/go/+/549256 (attn @bcmills). To avoid making a breaking change in go-winio, I would suggest doing something like this:

// SetFileBasicInfo sets times and attributes for a file.
func SetFileBasicInfo(f *os.File, bi *FileBasicInfo) error {
        var tmp = struct {
	      CreationTime, LastAccessTime, LastWriteTime, ChangeTime uint64
	      FileAttributes                                          uint32
	      _                                                       uint32 // padding
        }{bi.CreationTime, bi.LastAccessTime, bi.LastWriteTime, bi.ChangeTime, bi.FileAttributes} // This doesn't compile, but you get the idea.

	if err := windows.SetFileInformationByHandle(
		windows.Handle(f.Fd()),
		windows.FileBasicInfo,
		(*byte)(unsafe.Pointer(&tmp)),
		uint32(unsafe.Sizeof(tmp)),
	); err != nil {
		return &os.PathError{Op: "SetFileInformationByHandle", Path: f.Name(), Err: err}
	}
	runtime.KeepAlive(f)
	return nil
}

@dagood, could you fork go-winio, apply the suggested changes, update moby's go.mod and re-run the tests to see if this solves the issue?

[dagood]

Nice, that works! Thanks. Specifically, tried this:

	merge := func(t windows.Filetime) uint64 {
		return *(*uint64)(unsafe.Pointer(&t))
	}
	biAligned := struct {
		CreationTime, LastAccessTime, LastWriteTime, ChangeTime uint64
		FileAttributes                                          uint32
		_                                                       uint32 // padding
	}{
		merge(bi.CreationTime), merge(bi.LastAccessTime), merge(bi.LastWriteTime), merge(bi.ChangeTime),
		bi.FileAttributes,
		0,
	}

	if err := windows.SetFileInformationByHandle(
		windows.Handle(f.Fd()),
		windows.FileBasicInfo,
		(*byte)(unsafe.Pointer(&biAligned)),
		uint32(unsafe.Sizeof(biAligned)),

I suppose we can't say 100% conclusively that this is the cause just because CI passes (adding a fmt call causes it to pass, too) but the logic certainly makes sense. 🙂 I didn't notice the alignment effect of syscall.Filetime -> int64 in that CL--was focused on the _ uint32 (which was already present here) and the bad uintptr.

So, I suppose this was caused by the location of bi happening to be misaligned rather than aligned when built by go1.22rc1... and as far as I know, this could be caused by a huge variety of perfectly fine changes, so there isn't much point tracking that down.

Perhaps go-winio can also add tests that check the alignment of the structs as Go sees them vs. the actual C headers (or at least vs. hand-written values derived from the docs).

Closing: not a Go bug.

[dagood]

So, I suppose this was caused by the location of bi happening to be misaligned rather than aligned when built by go1.22rc1... and as far as I know, this could be caused by a huge variety of perfectly fine changes, so there isn't much point tracking that down.

Reading the nice 1.22 release notes, it looks like it's probably related to this:

... this change adjusts the size class boundaries of the memory allocator ...

A consequence of this change is that some objects' addresses that were previously always aligned to a 16 byte (or higher) boundary will now only be aligned to an 8 byte boundary. Some programs that use assembly instructions that require memory addresses to be more than 8-byte aligned and rely on the memory allocator's previous alignment behavior may break, but we expect such programs to be rare. Such programs may be built with GOEXPERIMENT=noallocheaders to revert to the old metadata layout and restore the previous alignment behavior, but package owners should update their assembly code to avoid the alignment assumption, as this workaround will be removed in a future release.

The numbers aren't quite the same, but it's the same pattern: this was code that relied on Go using 8 byte (or higher) alignment even though the struct itself only specifies 4 byte alignment.

Mentioning in this thread in case anyone finds this issue later without context, or if anyone already here was interested.