8000 syscall: fix dangling pointers in Windows' process attributes · golang/go@3a4f077 · GitHub
[go: up one dir, main page]
Skip to content

Commit 3a4f077

Browse files
qmuntalqmuntal
committed
syscall: fix dangling pointers in Windows' process attributes
Windows' _PROC_THREAD_ATTRIBUTE_LIST can contain pointers to memory owned by Go, but the GC is not aware of this. This can lead to the memory being freed while the _PROC_THREAD_ATTRIBUTE_LIST is still in use. This CL uses the same approach as in x/sys/windows to ensure that the attributes are not collected by the GC. Fixes #73170. Updates #73199. Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest Change-Id: I7dca8d386aed4c02fdcd4a631d0fa4dc5747a96f Reviewed-on: https://go-review.googlesource.com/c/go/+/663715 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: Keith Randall <khr@google.com>
1 parent 58b6891 commit 3a4f077

File tree

4 files changed

+68
-15
lines changed

4 files changed

+68
-15
lines changed

‎src/syscall/exec_windows.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -332,20 +332,20 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle
332332
defer DuplicateHandle(parentProcess, fd[i], 0, nil, 0, false, DUPLICATE_CLOSE_SOURCE)
333333
}
334334
}
335-
si := new(_STARTUPINFOEXW)
336-
si.ProcThreadAttributeList, err = newProcThreadAttributeList(2)
335+
procAttrList, err := newProcThreadAttributeList(2)
337336
if err != nil {
338337
return 0, 0, err
339338
}
340-
defer deleteProcThreadAttributeList(si.ProcThreadAttributeList)
339+
defer procAttrList.delete()
340+
si := new(_STARTUPINFOEXW)
341341
si.Cb = uint32(unsafe.Sizeof(*si))
342342
si.Flags = STARTF_USESTDHANDLES
343343
if sys.HideWindow {
344344
si.Flags |= STARTF_USESHOWWINDOW
345345
si.ShowWindow = SW_HIDE
346346
}
347347
if sys.ParentProcess != 0 {
348-
err = updateProcThreadAttribute(si.ProcThreadAttributeList, 0, _PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, unsafe.Pointer(&sys.ParentProcess), unsafe.Sizeof(sys.ParentProcess), nil, nil)
348+
err = procAttrList.update(_PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, unsafe.Pointer(&sys.ParentProcess), unsafe.Sizeof(sys.ParentProcess))
349349
if err != nil {
350350
return 0, 0, err
351351
}
@@ -371,7 +371,7 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle
371371

372372
// Do not accidentally inherit more than these handles.
373373
if willInheritHandles {
374-
err = updateProcThreadAttribute(si.ProcThreadAttributeList, 0, _PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0]), nil, nil)
374+
err = procAttrList.update(_PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0]))
375375
if err != nil {
376376
return 0, 0, err
377377
}
@@ -382,6 +382,7 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle
382382
return 0, 0, err
383383
}
384384

385+
si.ProcThreadAttributeList = procAttrList.list()
385386
pi := new(ProcessInformation)
386387
flags := sys.CreationFlags | CREATE_UNICODE_ENVIRONMENT | _EXTENDED_STARTUPINFO_PRESENT
387388
if sys.Token != 0 {

‎src/syscall/syscall_windows.go

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ func NewCallbackCDecl(fn any) uintptr {
287287
//sys GetCommandLine() (cmd *uint16) = kernel32.GetCommandLineW
288288
//sys CommandLineToArgv(cmd *uint16, argc *int32) (argv *[8192]*[8192]uint16, err error) [failretval==nil] = shell32.CommandLineToArgvW
289289
//sys LocalFree(hmem Handle) (handle Handle, err error) [failretval!=0]
290+
//sys localAlloc(flags uint32, length uint32) (ptr uintptr, err error) = kernel32.LocalAlloc
290291
//sys SetHandleInformation(handle Handle, mask uint32, flags uint32) (err error)
291292
//sys FlushFileBuffers(handle Handle) (err error)
292293
//sys GetFullPathName(path *uint16, buflen uint32, buf *uint16, fname **uint16) (n uint32, err error) = kernel32.GetFullPathNameW
@@ -1409,10 +1410,8 @@ func PostQueuedCompletionStatus(cphandle Handle, qty uint32, key uint32, overlap
14091410
return postQueuedCompletionStatus(cphandle, qty, uintptr(key), overlapped)
14101411
}
14111412

1412-
// newProcThreadAttributeList allocates new PROC_THREAD_ATTRIBUTE_LIST, with
1413-
// the requested maximum number of attributes, which must be cleaned up by
1414-
// deleteProcThreadAttributeList.
1415-
func newProcThreadAttributeList(maxAttrCount uint32) (*_PROC_THREAD_ATTRIBUTE_LIST, error) {
1413+
// newProcThreadAttributeList allocates a new [procThreadAttributeListContainer], with the requested maximum number of attributes.
1414+
func newProcThreadAttributeList(maxAttrCount uint32) (*procThreadAttributeListContainer, error) {
14161415
var size uintptr
14171416
err := initializeProcThreadAttributeList(nil, maxAttrCount, 0, &size)
14181417
if err != ERROR_INSUFFICIENT_BUFFER {
@@ -1421,13 +1420,38 @@ func newProcThreadAttributeList(maxAttrCount uint32) (*_PROC_THREAD_ATTRIBUTE_LI
14211420
}
14221421
return nil, err
14231422
}
1424-
// size is guaranteed to be ≥1 by initializeProcThreadAttributeList.
1425-
al := (*_PROC_THREAD_ATTRIBUTE_LIST)(unsafe.Pointer(&make([]byte, size)[0]))
1426-
err = initializeProcThreadAttributeList(al, maxAttrCount, 0, &size)
1423+
const LMEM_FIXED = 0
1424+
alloc, err := localAlloc(LMEM_FIXED, uint32(size))
14271425
if err != nil {
14281426
return nil, err
14291427
}
1430-
return al, nil
1428+
// size is guaranteed to be ≥1 by InitializeProcThreadAttributeList.
1429+
al := &procThreadAttributeListContainer{data: (*_PROC_THREAD_ATTRIBUTE_LIST)(unsafe.Pointer(alloc))}
1430+
err = initializeProcThreadAttributeList(al.data, maxAttrCount, 0, &size)
1431+
if err != nil {
1432+
return nil, err
1433+
}
1434+
al.pointers = make([]unsafe.Pointer, 0, maxAttrCount)
1435+
return al, err
1436+
}
1437+
1438+
// Update modifies the ProcThreadAttributeList using UpdateProcThreadAttribute.
1439+
func (al *procThreadAttributeListContainer) update(attribute uintptr, value unsafe.Pointer, size uintptr) error {
1440+
al.pointers = append(al.pointers, value)
1441+
return updateProcThreadAttribute(al.data, 0, attribute, value, size, nil, nil)
1442+
}
1443+
1444+
// Delete frees ProcThreadAttributeList's resources.
1445+
func (al *procThreadAttributeListContainer) delete() {
1446+
deleteProcThreadAttributeList(al.data)
1447+
LocalFree(Handle(unsafe.Pointer(al.data)))
1448+
al.data = nil
1449+
al.pointers = nil
1450+
}
1451+
1452+
// List returns the actual ProcThreadAttributeList to be passed to StartupInfoEx.
1453+
func (al *procThreadAttributeListContainer) list() *_PROC_THREAD_ATTRIBUTE_LIST {
1454+
return al.data
14311455
}
14321456

14331457
// RegEnumKeyEx enumerates the subkeys of an open registry key.

‎src/syscall/types_windows.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
package syscall
66

7+
import "unsafe"
8+
79
const (
810
// Windows errors.
911
ERROR_FILE_NOT_FOUND Errno = 2
@@ -496,8 +498,15 @@ type StartupInfo struct {
496498
StdErr Handle
497499
}
498500

499-
type _PROC_THREAD_ATTRIBUTE_LIST struct {
500-
_ [1]byte
501+
// _PROC_THREAD_ATTRIBUTE_LIST is a placeholder type to represent a the opaque PROC_THREAD_ATTRIBUTE_LIST.
502+
//
503+
// Manipulate this type only through [procThreadAttributeListContainer] to ensure proper handling of the
504+
// underlying memory. See https://g.dev/issue/73170.
505+
type _PROC_THREAD_ATTRIBUTE_LIST struct{}
506+
507+
type procThreadAttributeListContainer struct {
508+
data *_PROC_THREAD_ATTRIBUTE_LIST
509+
pointers []unsafe.Pointer
501510
}
502511

503512
const (

‎src/syscall/zsyscall_windows.go

Lines changed: 19 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
0