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 <[email protected]>
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Jason A. Donenfeld <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>

Author: zx2c4

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.

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

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")
+	}
+}

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 {