Closed zx2c4 closed 3 years ago
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
?
Alex
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
.
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
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.
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.
Change https://golang.org/cl/288298 mentions this issue: syscall: introduce SysProcAttr.AdditionalInheritedHandles on Windows
Change https://golang.org/cl/288300 mentions this issue: syscall: introduce SysProcAttr.ParentProcess on Windows
Change https://golang.org/cl/288299 mentions this issue: os: mark pipes returned by os.Pipe() as inheritable by default
Change https://golang.org/cl/288297 mentions this issue: syscall: restrict inherited handles on Windows
Change https://golang.org/cl/288296 mentions this issue: syscall: add support for proc thread attribute lists
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.
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.
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
@gopherbot add OS-Windows
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.
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
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.
Change https://golang.org/cl/323352 mentions this issue: doc/go1.17: mention new Windows SysProcAttr fields
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 (viaSysProcAttr.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:
SysProcAttr.AdditionalInheritedHandles
which takes a list of handles to add to that list of point (1).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