-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add ability to spawn Windows process with Proc Thread Attributes | Take 2 #114848
Add ability to spawn Windows process with Proc Thread Attributes | Take 2 #114848
Conversation
r? @ChrisDenton (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
ee8a884
to
e035a33
Compare
Okay the tracking issue has been created under #114854. |
Before bringing this to review I would like to revisit the conversation about the parallels between proc attributes and thread attributes. Since both support the attachment of attributes and the api is the same for both I think that the current implementation of this PR should be changed. I believe we can make this work with threads by ether:
I think the later is more practical since windows states that a ProcThreadAttributeList can be used to crate multiple process/threads. Second thing: Removing the |
I would tend to agree this is the better option but I think revisiting the old API would require a new ACPppropriate. It could explain the proposed public API for both Alternatively we could start with merging what we have here and leave discussion on an improved API to the tracking issue. |
@ChrisDenton I didn't exactly get what you were saying should I open a new ACP or not? For now, I wouldn't change the public interface, except maybe the documentation. |
If you're not changing the API from the original then I think it's fine to go ahead with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good to me but the docs could have a bit more explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lines in this file are sorted by doing a lowercase comparison. Hopefully this sorting will be better automated in the future.
@rustbot label -S-waiting-on-author +S-waiting-on-review |
Somebody has to test this on a windows target, im only on Unix so i can't test this. |
let mut proc_thread_attribute_list = | ||
ProcThreadAttributeList(vec![0u8; required_size as usize].into_boxed_slice()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be MaybeUninit
, but it's optional. Maybe adding fixme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the benifit of the MaybeUninit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaybeUninit
would allow the allocation to be faster, since we'd skip writing all 0's. Since we never directly read the memory, and the Win32 API is just fine with this memory being uninitialized (see example code here near the bottom of that page), we can do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the current impl?
library/std/src/process/tests.rs
Outdated
child.kill().unwrap(); | ||
parent.kill().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kill
calls will not be run if the test panics. This will result in the test process never exiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently not on my pc but I will fix that as soon as I get back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in 0e84161 I prevented the test from leaking the process if the spawn of the child
fails. But the call to wnic
could still fail if it doesn't find the process_id
or the binary simply doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the following snippet on Stack Overflow, it locates a parent process using the win32 API:
#include <stdio.h>
#include <windows.h>
#include <tlhelp32.h>
int main(int argc, char *argv[])
{
int pid = -1;
HANDLE h = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
PROCESSENTRY32 pe = { 0 };
pe.dwSize = sizeof(PROCESSENTRY32);
//assume first arg is the PID to get the PPID for, or use own PID
if (argc > 1) {
pid = atoi(argv[1]);
} else {
pid = GetCurrentProcessId();
}
if( Process32First(h, &pe)) {
do {
if (pe.th32ProcessID == pid) {
printf("PID: %i; PPID: %i\n", pid, pe.th32ParentProcessID);
}
} while( Process32Next(h, &pe));
}
CloseHandle(h);
}
Maybe this could be more stable?
All functions are loaded through Kernel32.dll
and are available since Windows XP and Windows Server 2003.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TyPR124 what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easiest way to ensure a process is killed would be to wrap the spawned Child
in a tuple struct that implements drop. E.g.:
struct Process(crate::process::Child);
impl Drop for Process {
fn drop(&mut self) {
let _ = self.0.kill();
}
}
let parent = Process(Command::new("cmd.exe").spawn().unwrap());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it's also useful to also use the winapi directly instead of wmic though. The wmic utility is marked as deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will try to test the win32 API version on a windows target as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is a mock-up implementation for now. If this is unwanted, I will revert the change tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this is looking ready to merge. Just one small thing then please squish your commits when you're ready.
You might also want to update your opening post to reflect the current state of this PR.
Okay, I will squash all commit now, or @TyPR124 should I keep yours at the base? |
Squish your commits but make sure that the commit message has |
7eb38af
to
6d6a167
Compare
This implements the ability to add arbitrary attributes to a command on Windows targets using a new `raw_attribute` method on the [`CommandExt`](https://doc.rust-lang.org/stable/std/os/windows/process/trait.CommandExt.html) trait. Setting these attributes provides extended configuration options for Windows processes. Co-authored-by: Tyler Ruckinger <t.ruckinger@gmail.com>
6d6a167
to
edefa8b
Compare
@ChrisDenton Is there anything left for me to do ? |
Nope, just waiting on me to take a final look at the code. Which I've now done, so let's see if CI is happy with it. @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9847c64): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 630.025s -> 630.448s (0.07%) |
…g/rust#114848 Signed-off-by: James Sturtevant <jstur@microsoft.com>
This is the second attempt to merge pull request #88193 into the standard library.
This PR implements the ability to add arbitrary attributes to a command on Windows targets using a new
raw_attribute
method on theCommandExt
trait.@TyPR124 and my main motivation behind adding this feature is to enable the support of pseudo terminals in the std library, but there are many more applications. A good starting point to get into this topic is to head over to the
Win32 API documentation
.