From 444254391f19087a15583a4b90ea84e7110e709d Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Sun, 28 Feb 2021 12:33:30 +0100 Subject: [PATCH] windows: treat proc thread attribute lists as unsafe.Pointers It turns out that the proc thread update function doesn't actually allocate new memory for its arguments and instead just copies the pointer values into the preallocated memory. Since we were allocating that memory as []byte, the garbage collector didn't scan it for pointers to Go allocations and freed them. We _could_ fix this by requiring that all users of this use runtime.KeepAlive for everything they pass to the update function, but that seems harder than necessary. Instead, we can just do the allocation as []unsafe.Pointer, which means the GC can operate as intended and not free these from beneath our feet. In order to ensure this remains true, we also add a test for this. Updates golang/go#44662. Change-Id: Iaa8b694a6682cc1876879632c7ba068e47b8666d Reviewed-on: https://go-review.googlesource.com/c/sys/+/297331 Trust: Jason A. Donenfeld Trust: Bryan C. Mills Run-TryBot: Jason A. Donenfeld TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor --- windows/exec_windows.go | 8 +++++--- windows/syscall_windows.go | 2 +- windows/syscall_windows_test.go | 34 +++++++++++++++++++++++++++++++++ windows/types_windows.go | 6 +++++- windows/zsyscall_windows.go | 2 +- 5 files changed, 46 insertions(+), 6 deletions(-) diff --git a/windows/exec_windows.go b/windows/exec_windows.go index 924775b8..9eb1fb63 100644 --- a/windows/exec_windows.go +++ b/windows/exec_windows.go @@ -111,7 +111,9 @@ func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeList, } return nil, err } - al := (*ProcThreadAttributeList)(unsafe.Pointer(&make([]byte, size)[0])) + const psize = unsafe.Sizeof(uintptr(0)) + // size is guaranteed to be ≥1 by InitializeProcThreadAttributeList. + al := (*ProcThreadAttributeList)(unsafe.Pointer(&make([]unsafe.Pointer, (size+psize-1)/psize)[0])) err = initializeProcThreadAttributeList(al, maxAttrCount, 0, &size) if err != nil { return nil, err @@ -120,8 +122,8 @@ func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeList, } // Update modifies the ProcThreadAttributeList using UpdateProcThreadAttribute. -func (al *ProcThreadAttributeList) Update(attribute uintptr, flags uint32, value unsafe.Pointer, size uintptr, prevValue uintptr, returnedSize *uintptr) error { - return updateProcThreadAttribute(al, flags, attribute, uintptr(value), size, prevValue, returnedSize) +func (al *ProcThreadAttributeList) Update(attribute uintptr, flags uint32, value unsafe.Pointer, size uintptr, prevValue unsafe.Pointer, returnedSize *uintptr) error { + return updateProcThreadAttribute(al, flags, attribute, value, size, prevValue, returnedSize) } // Delete frees ProcThreadAttributeList's resources. diff --git a/windows/syscall_windows.go b/windows/syscall_windows.go index 38d7d58b..376dd070 100644 --- a/windows/syscall_windows.go +++ b/windows/syscall_windows.go @@ -216,7 +216,7 @@ func NewCallbackCDecl(fn interface{}) uintptr { //sys CreateProcess(appName *uint16, commandLine *uint16, procSecurity *SecurityAttributes, threadSecurity *SecurityAttributes, inheritHandles bool, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation) (err error) = CreateProcessW //sys initializeProcThreadAttributeList(attrlist *ProcThreadAttributeList, attrcount uint32, flags uint32, size *uintptr) (err error) = InitializeProcThreadAttributeList //sys deleteProcThreadAttributeList(attrlist *ProcThreadAttributeList) = DeleteProcThreadAttributeList -//sys updateProcThreadAttribute(attrlist *ProcThreadAttributeList, flags uint32, attr uintptr, value uintptr, size uintptr, prevvalue uintptr, returnedsize *uintptr) (err error) = UpdateProcThreadAttribute +//sys updateProcThreadAttribute(attrlist *ProcThreadAttributeList, flags uint32, attr uintptr, value unsafe.Pointer, size uintptr, prevvalue unsafe.Pointer, returnedsize *uintptr) (err error) = UpdateProcThreadAttribute //sys OpenProcess(desiredAccess uint32, inheritHandle bool, processId uint32) (handle Handle, err error) //sys ShellExecute(hwnd Handle, verb *uint16, file *uint16, args *uint16, cwd *uint16, showCmd int32) (err error) [failretval<=32] = shell32.ShellExecuteW //sys GetWindowThreadProcessId(hwnd HWND, pid *uint32) (tid uint32, err error) = user32.GetWindowThreadProcessId diff --git a/windows/syscall_windows_test.go b/windows/syscall_windows_test.go index a3d5a873..34315460 100644 --- a/windows/syscall_windows_test.go +++ b/windows/syscall_windows_test.go @@ -16,6 +16,7 @@ import ( "strings" "syscall" "testing" + "time" "unsafe" "golang.org/x/sys/windows" @@ -502,3 +503,36 @@ func TestNTStatusConversion(t *testing.T) { t.Errorf("NTStatus.Errno = %q (0x%x); want %q (0x%x)", got.Error(), got, want.Error(), want) } } + +func TestProcThreadAttributeListPointers(t *testing.T) { + list, err := windows.NewProcThreadAttributeList(1) + if err != nil { + t.Errorf("unable to create ProcThreadAttributeList: %v", err) + } + done := make(chan struct{}) + fds := make([]syscall.Handle, 20) + runtime.SetFinalizer(&fds[0], func(*syscall.Handle) { + close(done) + }) + err = list.Update(windows.PROC_THREAD_ATTRIBUTE_HANDLE_LIST, 0, unsafe.Pointer(&fds[0]), uintptr(len(fds))*unsafe.Sizeof(fds[0]), nil, nil) + if err != nil { + list.Delete() + t.Errorf("unable to update ProcThreadAttributeList: %v", err) + return + } + runtime.GC() + runtime.GC() + select { + case <-done: + t.Error("ProcThreadAttributeList was garbage collected unexpectedly") + default: + } + list.Delete() + runtime.GC() + runtime.GC() + select { + case <-done: + case <-time.After(time.Second): + t.Error("ProcThreadAttributeList was not garbage collected after a second") + } +} diff --git a/windows/types_windows.go b/windows/types_windows.go index d2e0745e..abefd93a 100644 --- a/windows/types_windows.go +++ b/windows/types_windows.go @@ -912,7 +912,11 @@ type StartupInfoEx struct { // To create a *ProcThreadAttributeList, use NewProcThreadAttributeList, and // free its memory using ProcThreadAttributeList.Delete. type ProcThreadAttributeList struct { - _ [1]byte + // This is of type unsafe.Pointer, not of type byte or uintptr, because + // the contents of it is mostly a list of pointers, and in most cases, + // that's a list of pointers to Go-allocated objects. In order to keep + // the GC from collecting these objects, we declare this as unsafe.Pointer. + _ [1]unsafe.Pointer } type ProcessInformation struct { diff --git a/windows/zsyscall_windows.go b/windows/zsyscall_windows.go index 8b16cb35..6f16452c 100644 --- a/windows/zsyscall_windows.go +++ b/windows/zsyscall_windows.go @@ -2811,7 +2811,7 @@ func UnmapViewOfFile(addr uintptr) (err error) { return } -func updateProcThreadAttribute(attrlist *ProcThreadAttributeList, flags uint32, attr uintptr, value uintptr, size uintptr, prevvalue uintptr, returnedsize *uintptr) (err error) { +func updateProcThreadAttribute(attrlist *ProcThreadAttributeList, flags uint32, attr uintptr, value unsafe.Pointer, size uintptr, prevvalue unsafe.Pointer, returnedsize *uintptr) (err error) { r1, _, e1 := syscall.Syscall9(procUpdateProcThreadAttribute.Addr(), 7, uintptr(unsafe.Pointer(attrlist)), uintptr(flags), uintptr(attr), uintptr(value), uintptr(size), uintptr(prevvalue), uintptr(unsafe.Pointer(returnedsize)), 0, 0) if r1 == 0 { err = errnoErr(e1)