proposal: syscall: allow to pass STARTUPINFOEX to StartProcess Windows API

[alexbrainman]

I needed to pass STARTUPINFOEX structure to Windows StartProcess API. It is impossible to do without copying quite a bit of Go main lib code.

I propose we add new PROC_THREAD_ATTRIBUTE_LIST type to syscall package with this public API:

type PROC_THREAD_ATTRIBUTE_LIST [1]byte

func NewProcThreadAttributeList(maxattrcount uint32) (*PROC_THREAD_ATTRIBUTE_LIST, error)

func (al *PROC_THREAD_ATTRIBUTE_LIST) Release()

func UpdateProcThreadAttribute(attrlist *PROC_THREAD_ATTRIBUTE_LIST, flags uint32, attr uintptr, value uintptr, size uintptr, prevvalue uintptr, returnedsize *uintptr) (err error)

then we can pass new PROC_THREAD_ATTRIBUTE_LIST variable via new syscall.SysProcAttr.ProcThreadAttributeList field

type SysProcAttr struct {
        ...
        ProcThreadAttributeList *PROC_THREAD_ATTRIBUTE_LIST // if set, CreateProcess will be passed STARTUPINFOEX instead of STARTUPINFO with lpAttributeList set to ProcThreadAttributeList
}

This will allow me to build my program by using standard os/exec package.

I expect this new addition might be useful for other projects (for example #21085 and probably many others). But I don't intend add more code except this proposal.

You can see my proposed change at

https://go-review.googlesource.com/c/go/+/288272

Thank you for considering.

/cc @zx2c4 @bradfitz @mattn

Alex

[bradfitz]

I think you forgot to mention the key part: adding a new syscall.SysProcAttr field.

None of the existing ones are in SCREAMING_CASE, though:

https://golang.org/pkg/syscall/?GOOS=windows#SysProcAttr

Nor are any other types in syscall.

So how about defined it like:

type ProcThreadAttrList struct { _ [1]byte }
func (*ProcThreadAttrList) Release()
func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttrList, error)
etc

[zx2c4]

I actually think this is the wrong way to go about this. Instead of exposing that crazy structure, we should add fields to SysProcAttrs for the ones we want to support, such as PROC_THREAD_ATTRIBUTE_HANDLE_LIST and PROC_THREAD_ATTRIBUTE_PARENT_PROCESS.

Actually, I think we should take that a step further and always use PROC_THREAD_ATTRIBUTE_HANDLE_LIST when launching processes, in order to prevent handle leaks and prevent races with functions like ShellExecute. And we might even be able to remove that global lock we're using now, which would be great. To that end, I propose:

1. Always use PROC_THREAD_ATTRIBUTE_HANDLE_LIST for the handles we want to be inherited, automatically. No new API for this first point but rather making our existing logic more robust.
2. Add SysProcAttrs.AdditionalInheritedHandles which takes a list of handles to add to that list of point (1).
3. Add SysProcAttrs.ParentProcessHandle that adds PROC_THREAD_ATTRIBUTE_PARENT_PROCESS with that value.

I think this would cover your use cases well and also make things more robust for current users.

[alexbrainman]

I think you forgot to mention the key part: adding a new syscall.SysProcAttr field.

Indeed. I just updated my proposal. PTAL.

None of the existing ones are in SCREAMING_CASE, though:

PROC_THREAD_ATTRIBUTE_LIST is how it is spelled by Microsoft. It is impossible to google otherwise. But I am not married to this idea. I am fine to change my proposal.

Alex

[alexbrainman]

I actually think this is the wrong way to go about this. Instead of exposing that crazy structure, we should add fields to SysProcAttrs for the ones we want to support, such as PROC_THREAD_ATTRIBUTE_HANDLE_LIST and PROC_THREAD_ATTRIBUTE_PARENT_PROCESS.

I disagree. I propose to add single new field to SysProcAttrs. And that should cover all scenarios in the future. Once implemented, any user can implement any of the types supported by UpdateProcThreadAttribute Windows API without the need to hack standard library.

Actually, I think we should take that a step further and always use PROC_THREAD_ATTRIBUTE_HANDLE_LIST when launching processes, ...

This proposal is strictly about adding ability to pass STARTUPINFOEX to StartProcess.

Alex

[bradfitz]

PROC_THREAD_ATTRIBUTE_LIST is how it is spelled by Microsoft. It is impossible to google otherwise. But I am not married to this idea. I am fine to change my proposal.

It's fine to mention the Microsoft spelling in godoc. Google will handle the rest.

[alexbrainman]

It's fine to mention the Microsoft spelling in godoc. Google will handle the rest.

Names are important. And PROC_THREAD_ATTRIBUTE_LIST is clear without looking at the go doc. This name provides immediate meaning once I see it. Just like I see meaning of exported variable when its name starts with capital letter.

I, while I agree, it does not look pretty, that is what this thing is called.

I would like to hear others opinions before changing our mind.

Alex

[zx2c4]

I actually think this is the wrong way to go about this. Instead of exposing that crazy structure, we should add fields to SysProcAttrs for the ones we want to support, such as PROC_THREAD_ATTRIBUTE_HANDLE_LIST and PROC_THREAD_ATTRIBUTE_PARENT_PROCESS.

I disagree. I propose to add single new field to SysProcAttrs. And that should cover all scenarios in the future. Once implemented, any user can implement any of the types supported by UpdateProcThreadAttribute Windows API without the need to hack standard library.

What's the point of introducing a non-Go-like API to users when we already have a very nice abstraction over this? There's nothing special about these new attributes, except they're passed to CreateProcess yet-another-way because Microsoft has different generations of programmers who like to add new ways of doing things. In Go, though, nobody benefits from that. Plus, I see little benefit in supporting all of those flags. Some have weird gotchas. The two I mentioned are a reasonable start.

Actually, I think we should take that a step further and always use PROC_THREAD_ATTRIBUTE_HANDLE_LIST when launching processes, ...

This proposal is strictly about adding ability to pass STARTUPINFOEX to StartProcess.

In that case, I think this proposal is misguided and should be rejected. I'll open a new GitHub issue.

[alexbrainman]

What's the point of introducing a non-Go-like API to users when we already have a very nice abstraction over this?

This is syscall package. Syscall package provides access to OS. It is fine to use abstractions in our code. But I chose not to use abstractions in my proposal. Abstractions needs to be understood by users and documented. My proposal tried to stay as close to Windows API as possible instead.

Alex

[zx2c4]

What's the point of introducing a non-Go-like API to users when we already have a very nice abstraction over this?

This is syscall package. Syscall package provides access to OS. It is fine to use abstractions in our code. But I chose not to use abstractions in my proposal. Abstractions needs to be understood by users and documented. My proposal tried to stay as close to Windows API as possible instead.

That struct happens to be in syscall because that's where platform-specific structs go. But what we're actually talking about something that intimately interacts with and affects os.StartProcess. And for there we already have a very well-defined, easy-to-use, and most importantly stable way of passing additional Windows process attributes in, via SysProcAttr. Exposing some behemoth Microsoft structure without considering the interactions with the rest of os.StartProcess is a recipe for disaster and a huge burden on users. This proposal is very, very unwise.

[zx2c4]

Another issue with this is purely technical: inherited fds need need to be relative to the new parent, not to the original parent. There's no way Go can set up a proper process context given the API you propose.

My proposal in #44011 deals with this very well.

[zx2c4]

I spent another several hours evaluating at this, writing new code to test different schemes, and seeing what looked most coherent. I could not figure out a way to expose PROC_THREAD_ATTRIBUTE_LIST directly to os.StartProcess in a safe way where the rest of our existing logic there could continue to exist and evolve. It just doesn't work. There's a good reason why we never allowed the user to pass STARTUPINFO, let alone STARTUPINFOEX, raw like this before.

And semantically, allowing the user to pass PROC_THREAD_ATTRIBUTE_LIST like this, but not directly touch the other fields in STARTUPINFO just doesn't make sense.

But I get the desire to be able to play with low level structs for users who want these in syscall.*. However, SysProcAttr is just not the right way to do that. The right place for that is syscall.CreateProcess/windows.CreateProcess.

So here is what I propose:

1. We go with #44011 to expose things that make sense in a safe way to os.StartProcess.
2. We add STARTUPINFOEX, PROC_THREAD_ATTRIBUTE_LIST, and all of the various PROC_THREAD_ATTRIBUTE_* constants to x/sys/windows for users who want to call windows.CreateProcess directly with their own hacked-up STARTUPINFOEX.

Point (1) will help address your objective in a clean way. And point (2) will help give "extra power" to people doing crazy stuff, and it will add it in the place meant for this type of stuff -- x/sys.

It's worth noting that (2) has never been safe to use in the past, because of issues with leaking inheritable handles. However, the series I posted to #44011 fixes this issue definitively, which means we can safely have this type of "raw" functionality in x/sys/windows.

[gopherbot]

Change https://golang.org/cl/288412 mentions this issue: windows: add definitions and functions for ProcThreadAttributeList

[zx2c4]

I've just submitted a CL that implements part (2) of my previous comment.

[zx2c4]

@alexbrainman -- Because I have some hint of what your objective here is, I wrote some example code to show how to launch an unelevated process from an elevated one using the new definitions I just added to x/sys/windows. It works very well:

package main

import (
    "log"
    "path/filepath"
    "unsafe"

    "golang.org/x/sys/windows"
)

func main() {
    windowsDirectory, err := windows.GetWindowsDirectory()
    if err != nil {
        log.Fatalf("Could not find Windows directory: %v", err)
    }
    notepad := filepath.Join(windowsDirectory, "notepad.exe")
    notepad16, err := windows.UTF16PtrFromString(notepad)
    if err != nil {
        log.Fatalf("Could not perform string conversion %v", err)
    }

    shellWindow := windows.GetShellWindow()
    if shellWindow == 0 {
        log.Fatalf("Could not find shell window")
    }
    var shellPid uint32
    _, err = windows.GetWindowThreadProcessId(shellWindow, &shellPid)
    if err != nil {
        log.Fatalf("Could not get shell pid: %v", err)
    }
    shellProcess, err := windows.OpenProcess(windows.PROCESS_CREATE_PROCESS, false, shellPid)
    if err != nil {
        log.Fatalf("Could not open shell process: %v", err)
    }
    defer windows.CloseHandle(shellProcess)

    startupInfo := new(windows.StartupInfoEx)
    startupInfo.Cb = uint32(unsafe.Sizeof(*startupInfo))
    startupInfo.ProcThreadAttributeList, err = windows.NewProcThreadAttributeList(1)
    if err != nil {
        log.Fatalf("Could not create proc thread attribute list: %v", err)
    }
    err = startupInfo.ProcThreadAttributeList.Add(windows.PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, unsafe.Pointer(&shellProcess), unsafe.Sizeof(shellProcess))
    if err != nil {
        log.Fatalf("Could not set parent process handle: %v", err)
    }

    processInfo := new(windows.ProcessInformation)
    err = windows.CreateProcess(notepad16, notepad16, nil, nil, false, windows.CREATE_NEW_CONSOLE|windows.EXTENDED_STARTUPINFO_PRESENT, nil, nil, &startupInfo.StartupInfo, processInfo)
    if err != nil {
        log.Fatalf("Could not create process: %v", err)
    }
    windows.CloseHandle(processInfo.Process)
    windows.CloseHandle(processInfo.Thread)
}

[rsc]

Commented over on https://github.com/golang/go/issues/44011#issuecomment-772742684:

I understand the generality of #44005, but the big problem is that if we ever want to use STARTUPINFOEX ourselves (such as to fix the problems with pipes, as above), then having a user-supplied one means we have to find some way to merge the user-supplied one with our own changes, and we don't know which details the user means to override and which not.

The more limited API here in this issue [that is, #44011] seems like a more maintainable long-term approach in that we can make adjustments as needed as the details change in future Windows versions.

[rsc]

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

[networkimprov]

@gopherbot add OS-Windows

[alexbrainman]

@zx2c4

I wrote some example code to show how to launch an unelevated process from an elevated one using the new definitions I just added to x/sys/windows.

Yes. Your code looks similar to mine. I only also need to pass command line parameters to the new process. And I need to adjust new process environment.

Alex

[zx2c4]

@alexbrainman Could you review the CL for this, then, so that you can use that code in x/sys? https://golang.org/cl/288412 is a simple x/sys/windows CL.

[rsc]

Based on the discussion above (because letting the user pass a STARTUPINFOEX precludes the Go runtime from ever being able to control any of those fields itself), this seems like a likely decline.

[rsc]

Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group

[rsc]

No change in consensus, so declined. — rsc for the proposal review group

[techema]

The changes to add PROC_THREAD_ATTRIBUTE_LIST seemed to have broken the (os.exec) Cmd.Start() functionality on Windows. Please see https://github.com/golang/go/issues/49044 for details