Add a way to limit inheritable handles when spawning child process on Windows #75551
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Looking at related issues, it seems it might be a good idea to break the default behavior to "not inherit anything". I don't know if that would break anything or require some unstable flags. cc @retep998 for thoughts. |
|
Personally I would prefer to never automatically inherit handles and instead always explicitly specify the set of handles to be inherited. However there is the potential for that to be a breaking change, and the migration story would not be great if the only way around it was using an unstable API. It's also really hard to actually determine whether anyone would be affected without just deploying it live and seeing whether people complain about it breaking. |
retep998
added
T-libs
|
Thanks for the context! If I understand correctly, the eventual goal is to inherit nothing by default and remove the Mutex. However there is a real concern about breaking stable Rust programs without an easy fix. It seems the "inherit nothing" might need to wait for the stabilization of the API so stable Rust programs have a way to migrate. I renamed the API from to make future migration smoother. |
The API limits handles to inherit. The implementation follows the "Programmatically controlling which handles are inherited by new processes in Win32" blog from Microsoft: https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873 If the API is not called, the old behavior of inheriting all inheritable handles is not changed.
|
I don't know anything about Windows APIs and are thus not qualified to review this. @rust-lang/libs could anyone of you take this or do you know anyone who could? |
|
☔ The latest upstream changes (presumably #75979) made this pull request unmergeable. Please resolve the merge conflicts. |
Summary: Spawning processes turns out to be tricky. Python 2: - "fork & exec" in plain Python is potentially dangerous. See D22855986 (c35b808). Disabling GC might have solved it, but still seems fragile. - "close_fds=True" works on Windows if there is no redirection. - Does not work well with `disable_standard_handle_inheritability` from `hgmain`. We patched it. See `contrib/python2-winbuild/0002-windows-make-subprocess-work-with-non-inheritable-st.patch`. Python 3: - "subprocess" uses native code for "fork & exec". It's safer. - (>= 3.8) "close_fds=True" works on Windows even with redirection. - "subprocess" exposes options to tweak low-level details on Windows. Rust: - No "close_fds=True" support for both Windows and Unix. - Does not have the `disable_standard_handle_inheritability` issue on Windows. - Impossible to cleanly support "close_fds=True" on Windows with existing stdlib. rust-lang/rust#75551 attempts to add that to stdlib. D23124167 provides a short-term solution that can have corner cases. Mercurial: - `win32.spawndetached` uses raw Win32 APIs to spawn processes, bypassing the `subprocess` Python stdlib. - Its use of `CreateProcessA` is undesirable. We probably want `CreateProcessW` (unless `CreateProcessA` speaks utf-8 natively). We are still on Python 2 on Windows, and we'd need to spawn processes correctly from Rust anyway, and D23124167 kind of fills the missing feature of `close_fds=True` from Python. So let's expose the Rust APIs. The binding APIs closely match the Rust API. So when we migrate from Python to Rust, the translation is more straightforward. Reviewed By: DurhamG Differential Revision: D23124168 fbshipit-source-id: 94a404f19326e9b4cca7661da07a4b4c55bcc395
| @@ -94,6 +101,13 @@ struct DropGuard<'a> { | |||
| lock: &'a Mutex, | |||
| } | |||
|
|
|||
| #[derive(Default)] | |||
| struct ProcAttributeList { | |||
| buf: Vec<u8>, | |||
There was a problem hiding this comment.
This isn't explicitly mentioned in the Windows API documentation, but are there any alignment requirements for the ProcAttributeList? I would expect at least 8 or 16 byte alignment since that is what malloc gives you in C.
There was a problem hiding this comment.
If PROC_THREAD_ATTRIBUTE_LIST were to be defined as an extern type, then this would be a great example of allowing alignment on extern types. I'm not aware of any alignment requirements, but 8/16 byte alignment would be a safe assumption (as that is what HeapAlloc guarantees).
JohnCSimon
added
S-waiting-on-author
|
Ping from triage |
LeSeulArtichaut
added
the
S-waiting-on-author
|
pinging again from triage @quark-zju - can you please address the merge conflict or close this pr if you're not willing to continue on this PR? thank you! |
|
@quark-zju Ping from triage! I'm closing this due to inactivity. Feel free to reopen or create a new PR when you're ready to work on this again, thanks! |
crlf0710
added
S-inactive
This is basically an implementation requested by #73281. The API is opt-in. The default behavior remains unchanged.
I read some contributing guidelines about RFC or not, tracking issues, etc. I'm relatively new and open to suggestions about best practice. I think the implementation is relatively straightforward with little room for design discussion. An RFC feels too heavyweight in this case. I plan to add a tracking issue (and update the code to point to it) after some consensus on the feature / API names and parameters.