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

FileBasicInfo alignment is incorrect, leading to winio.SetFileBasicInfo returning "Invalid access to memory location." #311

Closed
dagood opened this issue Jan 12, 2024 · 1 comment

Comments

@dagood
Copy link
Member

dagood commented Jan 12, 2024

Code that calls SetFileBasicInfo(f *os.File, bi *FileBasicInfo) error can in some cases get an error like this:

SetFileInformationByHandle \\?\C:\Windows\SystemTemp\hcs2001018820\Files.$wcidirs$: Invalid access to memory location.

This is ERROR_NOACCESS, which can indicate a STATUS_DATATYPE_MISALIGNMENT error.

The reason is that winio's FileBasicInfo uses windows.Filetime, causing the struct alignment to be wrong. I can't directly prove the cause for sure (it's not trivial to repro with specific functions), but I opened golang/go#65069 to get some help with it, and I don't think there's any doubt.

Here's FileBasicInfo for reference:

go-winio/fileinfo.go

Lines 14 to 19 in bc421d9

// FileBasicInfo contains file access time and file attributes information.
type FileBasicInfo struct {
CreationTime, LastAccessTime, LastWriteTime, ChangeTime windows.Filetime
FileAttributes uint32
_ uint32 // padding
}

type Filetime struct {
	LowDateTime  uint32
	HighDateTime uint32
}

The biggest field of FileBasicInfo is uint32, so the struct as a whole is aligned on 32 bits.

But the input to SetFileInformationByHandle needs to be 64-bit aligned to be compatible with Windows' struct:

FileBasicInfo defines the time properties using LARGE_INTEGER, which is a uint64.

typedef struct _FILE_BASIC_INFORMATION {
  LARGE_INTEGER CreationTime;
  LARGE_INTEGER LastAccessTime;
  LARGE_INTEGER LastWriteTime;
  LARGE_INTEGER ChangeTime;
  ULONG         FileAttributes;
} FILE_BASIC_INFORMATION, *PFILE_BASIC_INFORMATION;

A workaround is to define a new struct with the right alignment and copy the data over before sending it to the syscall:

func SetFileBasicInfo(f *os.File, bi *FileBasicInfo) error {
	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)),
	); err != nil {
		return &os.PathError{Op: "SetFileInformationByHandle", Path: f.Name(), Err: err}
	}
	runtime.KeepAlive(f)
	return nil
}

It seems to me that something similar should also be added to GetFileBasicInfo. At least, I don't know any reason why the alignment problem wouldn't also eventually cause a problem for someone using that function, too.


This issue is making moby/moby's attempt to update to go1.22rc1 fail in dockerd: moby/moby#46982. I confirmed that applying the workaround above makes the update pass CI.

For some reason, go1.22rc1 aggravated this issue in moby's case, but I don't see any reason it wouldn't affect any Go version, depending on your luck. More info at golang/go#65069.

I wrote a test case that could make it easier to spot this kind of thing in PR review: https://gist.github.com/dagood/3cd4207c1f12608f1cca2183af143715.

@dagood
Copy link
Member Author

dagood commented Jan 16, 2024

Fixed by #312

@dagood dagood closed this as completed Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant