syscall: add additional fields to SysProcAttr on Windows #44011
Comments
zx2c4
changed the title
I don't need this functionality now. I don't plan to work on this feature.
How do you propose to determine the scenario of 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 |
Correct. Reading Jason's proposal, I get impression that he believes that only PROC_THREAD_ATTRIBUTE_HANDLE_LIST and PROC_THREAD_ATTRIBUTE_PARENT_PROCESS 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 |
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.
Using zero, yes, matching Go's convention of having uninitialized structs being good-to-go. That will be documented.
Good.
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. |
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: |
|
Change https://golang.org/cl/288300 mentions this issue: |
|
Change https://golang.org/cl/288299 mentions this issue: |
|
Change https://golang.org/cl/288297 mentions this issue: |
|
Change https://golang.org/cl/288296 mentions this issue: |
|
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 |
|
@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. |
|
No change in consensus, so accepted. 🎉 |
rsc
changed the title
Great. https://go-review.googlesource.com/c/go/+/288300/6 and its relation chain contains the implementation. |
This will allow us to pass additional attributes when starting processes. Updates #44011. Change-Id: I4af365c5544a6d421830f247593ec970200e5e03 Reviewed-on: https://go-review.googlesource.com/c/go/+/288296 Trust: Jason A. Donenfeld <Jason@zx2c4.com> Trust: Alex Brainman <alex.brainman@gmail.com> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Windows does not have CLOEXEC, but rather handles are marked explicitly for being inherited by new processes. This can cause problems when different Windows functions create new processes from different threads. syscall.StartProcess has traditionally used a mutex to prevent races with itself, but this doesn't handle races with other win32 functions. Fortunately there's a solution: PROC_THREAD_ATTRIBUTE_HANDLE_LIST allows us to pass the entire list of handles that we want to be inherited. This lets us get rid of the mutex and also makes process creation safe across the Go runtime, no matter the context. Updates #44011. Change-Id: Ia3424cd2ec64868849cbd6cbb5b0d765224bf4ab Reviewed-on: https://go-review.googlesource.com/c/go/+/288297 Trust: Jason A. Donenfeld <Jason@zx2c4.com> Trust: Alex Brainman <alex.brainman@gmail.com> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
This allows users to specify handles that they explicitly want to be inherited by the new process. These handles must already be marked as inheritable. Updates #44011. Updates #21085. Change-Id: Ib18322e7dc2909e68c4209e80385492804fa15d3 Reviewed-on: https://go-review.googlesource.com/c/go/+/288298 Trust: Jason A. Donenfeld <Jason@zx2c4.com> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Now that we don't automatically pass all inheritable handles to new processes, we can make pipes returned by os.Pipe() inheritable, just like they are on Unix. This then allows them to be passed through the SysProcAttr.AdditionalInheritedHandles parameter simply. Updates #44011. Fixes #21085. Change-Id: I8eae329fbc74f9dc7962136fa9aae8fb66879751 Reviewed-on: https://go-review.googlesource.com/c/go/+/288299 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>
|
Change https://golang.org/cl/323352 mentions this issue: |
For #44011 For #44513 Change-Id: I512466f2e775e36098eb36ca7ef82333cd9e632a Reviewed-on: https://go-review.googlesource.com/c/go/+/323352 Trust: Ian Lance Taylor <iant@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
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.AdditionalInheritedHandleswhich takes a list of handles to add to that list of point (1).SysProcAttr.ParentProcessHandlethat 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.AdditionalInheritedHandlesSysProcAttr.ParentProcessHandleIf 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
The text was updated successfully, but these errors were encountered: