syscall: add additional fields to SysProcAttr on Windows

[zx2c4]

There are two additional attributes that would be useful to expose to the user in SysProcAttr: PROC_THREAD_ATTRIBUTE_HANDLE_LIST (via SysProcAttr.AdditionalInheritedHandles) and PROC_THREAD_ATTRIBUTE_PARENT_PROCESS (via SysProcAttr.ParentProcessHandle), documented here:

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

Microsoft added these at a later point (Windows Vista) than the other attributes that we already support in StartProcess/SysProcAttr, so internally there's a slightly different way of passing these to CreateProcess, but that's just a weird implementation quirk that has no bearing on the Go user.

PROC_THREAD_ATTRIBUTE_HANDLE_LIST in particular has special significance in that we actually should already be using it unconditionally for basic reliability. Specifically, we should 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 to hack around the absence of PROC_THREAD_ATTRIBUTE_HANDLE_LIST, which would be great.

To that end, I propose the following 3 steps:

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. This is an important reliability fix.
2. Add SysProcAttr.AdditionalInheritedHandles which takes a list of handles to add to that list of point (1).
3. Add SysProcAttr.ParentProcessHandle that adds PROC_THREAD_ATTRIBUTE_PARENT_PROCESS with that value.

Point (1) is a bug/reliability fix that doesn't require the proposals process, so I'll submit a boring CL for that independent of this proposal. Therefore this proposal concerns adding:

• SysProcAttr.AdditionalInheritedHandles
• SysProcAttr.ParentProcessHandle

If you think there are additional attributes we should expose via SysProcAttr, I'm happy to add those to this proposal too.

This is an alternative to #44005, which in my opinion would be big mistake.

Cc @mattn @bradfitz @alexbrainman

[alexbrainman]

• SysProcAttr.AdditionalInheritedHandles

I don't need this functionality now. I don't plan to work on this feature.

• SysProcAttr.ParentProcessHandle

How do you propose to determine the scenario of I don't want to change my new process parent? I assume you will use 0 to indicate this scenario. Is it possible for parent process handle to be 0? Anyway this need to be clearly documented.

So having access to SysProcAttr.ParentProcessHandle would solve my particular problem.

But #44005 provides general solution. And #44005 is not that complicated, and it is all based on current Microsoft official documentation. So the question is, should we add #44005 once and for all, or do we add new SysProcAttr field / fields when someone needs one of many listed on

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

?

Alex

[ianlancetaylor]

If we adopt #44005 then we can support pretty much any case. If we adopt this proposal we will have to add cases over time. #44005 also seems more akin to the approach we've taken on GNU/Linux, where we have fields like Cloneflags and Unshareflags and AmbientCaps.

[alexbrainman]

If we adopt #44005 then we can support pretty much any case. If we adopt this proposal we will have to add cases over time.

Correct.

Reading Jason's proposal, I get impression that he believes that only PROC_THREAD_ATTRIBUTE_HANDLE_LIST and PROC_THREAD_ATTRIBUTE_PARENT_PROCESS would be useful to expose. I don't know how correct he is. Only time will tell.

And, it appears to me, Microsoft can even add more options that you can pass to UpdateProcThreadAttribute Windows API over time (if they discover that CreateProcess Windows API require some new parameters). #44005 will just let users handle that scenario without talking to Go Team.

Alex

[zx2c4]

• SysProcAttr.AdditionalInheritedHandles

I don't need this functionality now. I don't plan to work on this feature.

But the Go runtime should unconditionally be using PROC_THREAD_ATTRIBUTE_HANDLE_LIST already. Allowing people to add to that list will be an important part of getting weird pipes working, per that other thread. So this is something that I'll be adding no matter what.

• SysProcAttr.ParentProcessHandle

How do you propose to determine the scenario of I don't want to change my new process parent? I assume you will use 0 to indicate this scenario. Is it possible for parent process handle to be 0? Anyway this need to be clearly documented.

Using zero, yes, matching Go's convention of having uninitialized structs being good-to-go. That will be documented.

So having access to SysProcAttr.ParentProcessHandle would solve my particular problem.

Good.

But #44005 provides general solution. And #44005 is not that complicated, and it is all based on current Microsoft official documentation. So the question is, should we add #44005 once and for all, or do we add new SysProcAttr field / fields when someone needs one of many listed on

We definitely do not want to expose that directly as part of our API to os.StartProcess. The right way to do this is to expose them one by one as they're needed and individually evaluated.

You can see that's the case by the fact that os.StartProcess is going to need to be using PROC_THREAD_ATTRIBUTE_HANDLE_LIST regardless of anything else use-provided, which means if we went with the bad #44005 approach, we'd all of the sudden be required to do weird merging of structs. That's not a headache I want to take on. Plus the API then becomes awful to use.

[zx2c4]

And, it appears to me, Microsoft can even add more options that you can pass to UpdateProcThreadAttribute Windows API over time (if they discover that CreateProcess Windows API require some new parameters). #44005 will just let users handle that scenario without talking to Go Team.

This is a recipe for disaster. We're talking about os.StartProcess here. We need for the parameters passed to that to be coherent with the rest of Go. Doing a freeforall not only leads to a bad-to-use API and breaks the current convention with how things are done, but also means we're opening ourselves up to future instability of os.StartProcess.

[gopherbot]

Change https://golang.org/cl/288298 mentions this issue: syscall: introduce SysProcAttr.AdditionalInheritedHandles on Windows

[gopherbot]

Change https://golang.org/cl/288300 mentions this issue: syscall: introduce SysProcAttr.ParentProcess on Windows

[gopherbot]

Change https://golang.org/cl/288299 mentions this issue: os: mark pipes returned by os.Pipe() as inheritable by default

[gopherbot]

Change https://golang.org/cl/288297 mentions this issue: syscall: restrict inherited handles on Windows

[gopherbot]

Change https://golang.org/cl/288296 mentions this issue: syscall: add support for proc thread attribute lists

[zx2c4]

I just posted a series of 5 step-by-step commits that add the ability to change the process parent and the ability for pipes and other things to be inherited in a SAFE way that doesn't interfere with the go runtime. They also make inheritability semantics of the runtime a lot more reliable than otherwise.

The series also deals with the complicated interaction between setting the parent process and the existing logic we have for passing on stdin/out/err, in order to avoid undefined behavior in the child process when it attempts to operate on bogus handles.

And, this finally fixes #21085 properly, after all these years.

Finally, the series keeps with the existing conventions for exposing the various flags in StartupInfo with an easy to use and safe API.

In my testing it appears to work well. Please take a look.

[rsc]

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

[zx2c4]

I'm now shipping this series as part of wireguard-windows 0.3.5, so it's receiving some real world testing. So far no bug reports related to it.

[rsc]

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

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[zx2c4]

No change in consensus, so accepted.
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Great. https://go-review.googlesource.com/c/go/+/288300/6 and its relation chain contains the implementation.

[gopherbot]

Change https://golang.org/cl/323352 mentions this issue: doc/go1.17: mention new Windows SysProcAttr fields