From 6ca3eb03dfc2ec15b5cb1a66e77caadc71075763 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Wed, 10 Mar 2021 07:33:27 -0700 Subject: [PATCH] windows: use Go-managed pointer list for ProcThreadAttributeList It turns out that if you write Go pointers to Go memory, the Go compiler must be involved so that it generates various calls to the GC in the process. Letting Windows write Go pointers to Go memory violated this. We fix this by having all the Windows-managed memory be just a boring []byte blob. Then, in order to prevent the GC from prematurely cleaning up the pointers referenced by that []byte blob, or in the future moving memory and attempting to fix up pointers, we copy the data to the Windows heap and then maintain a little array of pointers that have been used. Every time the Update function is called with a new pointer, we make a copy and append it to the list. Then, on Delete, we free the pointers from the Windows heap. Updates golang/go#44900. Change-Id: I42340a93fd9f6b8d10340634cf833fd4559a5f4f Reviewed-on: https://go-review.googlesource.com/c/sys/+/300369 Trust: Jason A. Donenfeld Run-TryBot: Jason A. Donenfeld TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor --- windows/exec_windows.go | 47 ++++++++++++++++++++++++++------- windows/syscall_windows_test.go | 34 ------------------------ windows/types_windows.go | 17 ++++++------ 3 files changed, 47 insertions(+), 51 deletions(-) diff --git a/windows/exec_windows.go b/windows/exec_windows.go index a020caee..7a11e83b 100644 --- a/windows/exec_windows.go +++ b/windows/exec_windows.go @@ -9,6 +9,8 @@ package windows import ( errorspkg "errors" "unsafe" + + "golang.org/x/sys/internal/unsafeheader" ) // EscapeArg rewrites command line argument s as prescribed @@ -135,8 +137,8 @@ func FullPath(name string) (path string, err error) { } } -// NewProcThreadAttributeList allocates a new ProcThreadAttributeList, with the requested maximum number of attributes. -func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeList, error) { +// NewProcThreadAttributeList allocates a new ProcThreadAttributeListContainer, with the requested maximum number of attributes. +func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeListContainer, error) { var size uintptr err := initializeProcThreadAttributeList(nil, maxAttrCount, 0, &size) if err != ERROR_INSUFFICIENT_BUFFER { @@ -145,10 +147,9 @@ func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeList, } return nil, err } - 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) + al := &ProcThreadAttributeListContainer{data: (*ProcThreadAttributeList)(unsafe.Pointer(&make([]byte, size)[0]))} + err = initializeProcThreadAttributeList(al.data, maxAttrCount, 0, &size) if err != nil { return nil, err } @@ -156,11 +157,39 @@ 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 unsafe.Pointer, returnedSize *uintptr) error { - return updateProcThreadAttribute(al, flags, attribute, value, size, prevValue, returnedSize) +// Note that the value passed to this function will be copied into memory +// allocated by LocalAlloc, the contents of which should not contain any +// Go-managed pointers, even if the passed value itself is a Go-managed +// pointer. +func (al *ProcThreadAttributeListContainer) Update(attribute uintptr, value unsafe.Pointer, size uintptr) error { + alloc, err := LocalAlloc(LMEM_FIXED, uint32(size)) + if err != nil { + return err + } + var src, dst []byte + hdr := (*unsafeheader.Slice)(unsafe.Pointer(&src)) + hdr.Data = value + hdr.Cap = int(size) + hdr.Len = int(size) + hdr = (*unsafeheader.Slice)(unsafe.Pointer(&dst)) + hdr.Data = unsafe.Pointer(alloc) + hdr.Cap = int(size) + hdr.Len = int(size) + copy(dst, src) + al.heapAllocations = append(al.heapAllocations, alloc) + return updateProcThreadAttribute(al.data, 0, attribute, unsafe.Pointer(alloc), size, nil, nil) } // Delete frees ProcThreadAttributeList's resources. -func (al *ProcThreadAttributeList) Delete() { - deleteProcThreadAttributeList(al) +func (al *ProcThreadAttributeListContainer) Delete() { + deleteProcThreadAttributeList(al.data) + for i := range al.heapAllocations { + LocalFree(Handle(al.heapAllocations[i])) + } + al.heapAllocations = nil +} + +// List returns the actual ProcThreadAttributeList to be passed to StartupInfoEx. +func (al *ProcThreadAttributeListContainer) List() *ProcThreadAttributeList { + return al.data } diff --git a/windows/syscall_windows_test.go b/windows/syscall_windows_test.go index d6571b71..0aaa0a33 100644 --- a/windows/syscall_windows_test.go +++ b/windows/syscall_windows_test.go @@ -18,7 +18,6 @@ import ( "strings" "syscall" "testing" - "time" "unsafe" "golang.org/x/sys/windows" @@ -506,39 +505,6 @@ func TestNTStatusConversion(t *testing.T) { } } -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") - } -} - func TestPEBFilePath(t *testing.T) { peb := windows.RtlGetCurrentPeb() if peb == nil || peb.Ldr == nil { diff --git a/windows/types_windows.go b/windows/types_windows.go index 23fe18ec..1f733398 100644 --- a/windows/types_windows.go +++ b/windows/types_windows.go @@ -909,14 +909,15 @@ type StartupInfoEx struct { // ProcThreadAttributeList is a placeholder type to represent a PROC_THREAD_ATTRIBUTE_LIST. // -// To create a *ProcThreadAttributeList, use NewProcThreadAttributeList, and -// free its memory using ProcThreadAttributeList.Delete. -type ProcThreadAttributeList struct { - // 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 +// To create a *ProcThreadAttributeList, use NewProcThreadAttributeList, update +// it with ProcThreadAttributeListContainer.Update, free its memory using +// ProcThreadAttributeListContainer.Delete, and access the list itself using +// ProcThreadAttributeListContainer.List. +type ProcThreadAttributeList struct{} + +type ProcThreadAttributeListContainer struct { + data *ProcThreadAttributeList + heapAllocations []uintptr } type ProcessInformation struct {