fileinfo: internally fix FileBasicInfo memory alignment (#312)

* fileinfo: internally fix FileBasicInfo memory alignment

Signed-off-by: Davis Goodin <dagood@microsoft.com>

* Update test with review feedback

Remove unused winName.

Extract more into Windows alignment consts to repeat less.

Document reason for having multiple alignment consts for the same value.

Signed-off-by: Davis Goodin <dagood@microsoft.com>

---------

Signed-off-by: Davis Goodin <dagood@microsoft.com>
(cherry picked from commit 008bc6e)
Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>

fileinfo.go

@@ -1,3 +1,4 @@
+//go:build windows
 // +build windows
 
 package winio
@@ -17,19 +18,43 @@ type FileBasicInfo struct {
 	pad                                                     uint32 // padding
 }
 
+// alignedFileBasicInfo is a FileBasicInfo, but aligned to uint64 by containing
+// uint64 rather than windows.Filetime. Filetime contains two uint32s. uint64
+// alignment is necessary to pass this as FILE_BASIC_INFO.
+type alignedFileBasicInfo struct {
+	CreationTime, LastAccessTime, LastWriteTime, ChangeTime uint64
+	FileAttributes                                          uint32
+	_                                                       uint32 // padding
+}
+
 // GetFileBasicInfo retrieves times and attributes for a file.
 func GetFileBasicInfo(f *os.File) (*FileBasicInfo, error) {
-	bi := &FileBasicInfo{}
-	if err := windows.GetFileInformationByHandleEx(windows.Handle(f.Fd()), windows.FileBasicInfo, (*byte)(unsafe.Pointer(bi)), uint32(unsafe.Sizeof(*bi))); err != nil {
+	bi := &alignedFileBasicInfo{}
+	if err := windows.GetFileInformationByHandleEx(
+		windows.Handle(f.Fd()),
+		windows.FileBasicInfo,
+		(*byte)(unsafe.Pointer(bi)),
+		uint32(unsafe.Sizeof(*bi)),
+	); err != nil {
 		return nil, &os.PathError{Op: "GetFileInformationByHandleEx", Path: f.Name(), Err: err}
 	}
 	runtime.KeepAlive(f)
-	return bi, nil
+	// Reinterpret the alignedFileBasicInfo as a FileBasicInfo so it matches the
+	// public API of this module. The data may be unnecessarily aligned.
+	return (*FileBasicInfo)(unsafe.Pointer(bi)), nil
 }
 
 // SetFileBasicInfo sets times and attributes for a file.
 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 {
+	// Create an alignedFileBasicInfo based on a FileBasicInfo. The copy is
+	// suitable to pass to GetFileInformationByHandleEx.
+	biAligned := *(*alignedFileBasicInfo)(unsafe.Pointer(bi))
+	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)

fileinfo_test.go

@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"os"
 	"testing"
+	"unsafe"
 
 	"golang.org/x/sys/windows"
 )
@@ -132,3 +133,54 @@ func TestGetFileStandardInfo_Directory(t *testing.T) {
 	}
 	checkFileStandardInfo(t, info, expectedFileInfo)
 }
+
+// TestFileInfoStructAlignment checks that the alignment of Go fileinfo structs
+// match what is expected by the Windows API.
+func TestFileInfoStructAlignment(t *testing.T) {
+	//nolint:revive // SNAKE_CASE is not idiomatic in Go, but aligned with Win32 API.
+	const (
+		// The alignment of various types, as named in the Windows APIs. When
+		// deciding on an expectedAlignment for a struct's test case, use the
+		// type of the largest field in the struct as written in the Windows
+		// docs. This is intended to help reviewers by allowing them to first
+		// check that a new align* const is correct, then independently check
+		// that the test case is correct, rather than all at once.
+		alignLARGE_INTEGER = unsafe.Alignof(uint64(0))
+		alignULONGLONG     = unsafe.Alignof(uint64(0))
+	)
+	tests := []struct {
+		name              string
+		actualAlign       uintptr
+		actualSize        uintptr
+		expectedAlignment uintptr
+	}{
+		{
+			// alignedFileBasicInfo is passed to the Windows API rather than FileBasicInfo.
+			"alignedFileBasicInfo", unsafe.Alignof(alignedFileBasicInfo{}), unsafe.Sizeof(alignedFileBasicInfo{}),
+			// https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_basic_info
+			alignLARGE_INTEGER,
+		},
+		{
+			"FileStandardInfo", unsafe.Alignof(FileStandardInfo{}), unsafe.Sizeof(FileStandardInfo{}),
+			// https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_standard_info
+			alignLARGE_INTEGER,
+		},
+		{
+			"FileIDInfo", unsafe.Alignof(FileIDInfo{}), unsafe.Sizeof(FileIDInfo{}),
+			// https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_info
+			alignULONGLONG,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.actualAlign != tt.expectedAlignment {
+				t.Errorf("alignment mismatch: actual %d, expected %d", tt.actualAlign, tt.expectedAlignment)
+			}
+			if r := tt.actualSize % tt.expectedAlignment; r != 0 {
+				t.Errorf(
+					"size is not a multiple of alignment: size %% alignment (%d %% %d) is %d, expected 0",
+					tt.actualSize, tt.expectedAlignment, r)
+			}
+		})
+	}
+}