Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
alexbrainman opened this issue Jan 30, 2021 · 23 comments
Closed

Comments

@alexbrainman
Copy link
Member

alexbrainman commented Jan 30, 2021

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

@gopherbot gopherbot added this to the Proposal milestone Jan 30, 2021
@bradfitz
Copy link
Contributor

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
Copy link
Contributor

zx2c4 commented Jan 30, 2021

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

zx2c4 commented Jan 30, 2021

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
Copy link
Member Author

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

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 31, 2021
@zx2c4
Copy link
Contributor

zx2c4 commented Jan 31, 2021

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
Copy link
Contributor

zx2c4 commented Jan 31, 2021

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
Copy link
Contributor

zx2c4 commented Jan 31, 2021

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 syscall: add additional fields to SysProcAttr on Windows #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
Copy link

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

@zx2c4
Copy link
Contributor

zx2c4 commented Jan 31, 2021

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

@zx2c4
Copy link
Contributor

zx2c4 commented Jan 31, 2021

@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:

image

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
Copy link
Contributor

rsc commented Feb 3, 2021

Commented over on #44011 (comment):

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 rsc moved this from Incoming to Active in Proposals (old) Feb 3, 2021
@rsc
Copy link
Contributor

rsc commented Feb 3, 2021

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
Copy link

@gopherbot add OS-Windows

@alexbrainman
Copy link
Member Author

@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
Copy link
Contributor

zx2c4 commented Feb 4, 2021

@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
Copy link
Contributor

rsc commented Feb 10, 2021

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
Copy link
Contributor

rsc commented Feb 10, 2021

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

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Feb 10, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Feb 17, 2021
@rsc
Copy link
Contributor

rsc commented Feb 17, 2021

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

@rsc rsc closed this as completed Feb 17, 2021
gopherbot pushed a commit to golang/sys that referenced this issue Feb 26, 2021
The bulk of the documentation for these is in:

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute

This allows creating processes from scratch that use advanced options,
such as changing the process parent.

Fixes golang/go#44005.

Change-Id: Id5cc5400541e57710b9e888cd37ef4f925d510fe
Reviewed-on: https://go-review.googlesource.com/c/sys/+/288412
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Trust: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@techema
Copy link

techema commented Oct 18, 2021

The changes to add PROC_THREAD_ATTRIBUTE_LIST seemed to have broken the (os.exec) Cmd.Start() functionality on Windows.
Please see #49044 for details

@golang golang locked and limited conversation to collaborators Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants