search

microsoft / go-winio

Win32 IO-related utilities for Go
MIT License
939 stars 181 forks source link

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

Closed dagood closed 6 months ago

dagood commented 6 months ago

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 https://github.com/golang/go/issues/65069 to get some help with it, and I don't think there's any doubt.

Here's FileBasicInfo for reference:

https://github.com/microsoft/go-winio/blob/bc421d9108ade8ae2f76c8ef3e90cd4ee690e2c0/fileinfo.go#L14-L19

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: https://github.com/moby/moby/pull/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 https://github.com/golang/go/issues/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 commented 6 months ago

Fixed by https://github.com/microsoft/go-winio/pull/312