Add ability to spawn Windows process with Proc Thread Attributes

[TyPR124]

This adds a Windows-specific extension method on Command that allows users to include proc thread attributes. The test uses the PROC_THREAD_ATTRIBUTE_PARENT_PROCESS attribute to spawn a process with a custom parent process, and then verifies the child's parent matches the expected parent. That seemed to me the easiest way to test.

Without this, it is not possible to implement anything requiring these attributes without re-implementing Command (and friends) from scratch as a separate crate.

In particular, I would like to use this to make use of the Windows ConPTY API. I've already succesfully implemented this in my own library, but don't like the thought of having to maintain my own copy of what is already in the standard library, potentially missing any future bug fixes or security updates.

This addition requires a minimum of Windows Vista or Server 2008, which as far as I understand should now be acceptable to add to the standard library since XP support is not required any longer.

The exposed API is pretty much the bare minimum to make this work, but it may be possible to improve the API in the future.

I can open a tracking issue and update the PR with that in a bit.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[ChrisDenton]

I would definitely love to see more of CreateProcessW exposed to users.

The exposed API is pretty much the bare minimum to make this work, but it may be possible to improve the API in the future.

Hm, it does feel like Command should ideally take ownership of the value somehow.

Other than that it would be good to have safe (or at least safer) APIs for common operations. E.g. setting the parent process, setting exploit mitigations and restricting inherited handles.

[ChrisDenton]

Does this need to be Pin? Surely the Vec itself can be moved around freely?

[TyPR124]

The Pin prevents us from calling (for example) vec.push() or doing anything that may re-allocate (and move) the underlying data. I'm not 100% sure it is necessary to not move the data, as I don't know the internal workings of these Initialize/Update/Delete functions, but it seemed the right choice to me since there is at least no good reason to move or grow the data after that point.

The Pin doesn't prevent us from moving the Vec itself (after all, we must move it out of this function to return it), just the data it points to.

[bjorn3]

[u8] is Unpin, so Pin<Vec<u8>> so both Pin::into_inner and DerefMut are implemented allowing you to move the inner bytes anyway.

https://doc.rust-lang.org/stable/std/pin/struct.Pin.html#method.into_inner

[ChrisDenton]

I think perhaps a boxed slice would be enough to prevent accidentally reallocating? Or it might be a good idea to fully contain the owned data. So have an API like:

struct ProcThreadAttributeList(Vec<u8>); // or Box<[T]>
impl ProcThreadAttributeList {
    fn new(attribute_count: u32) -> io::Result<Self> { ... }
    unsafe fn update(&mut self, attribute: usize, value_ptr: *mut c_void, value_size: u32) -> io::Result<()> { ... }
}
impl Drop for ProcThreadAttributeList { ... }

That way there is no reason to touch the inner data.

[TyPR124]

@ChrisDenton I could do that, just seemed like overkill to me when I was writing it.

@bjorn3 You could copy the bytes, or overwrite them, but you can't move them. let mut v = Pin::new(vec![]); v.push(()); does not compile. Unless I'm just being really dumb right now. You are right about Pin::into_inner but calling that explicitly indicates it is now okay to move the underlying data (which it might be anyway, but, again, I'm not certain).

[TyPR124]

Hm, it does feel like Command should ideally take ownership of the value somehow.

I think I agree. I am considering making the method generic over a T: Sized + Any, in which case we could grab the size ourselves and store the T in some Box<dyn Any>, making it slightly easier on the user. A couple concerns I have about that are:

• Do we really need to allocate for every attribute?
• Do we really need ownership of the generic type? Are there any situations where the user might need to retain ownership?
  • I'm guessing that any value passed in here would always be Copy so this shouldn't be an issue, but I'm not certain of that.

That said, the more I think about it, the more I tend to think taking ownership would be the right choice. But I'd like to hear if others have any feedback here first.

Other than that it would be good to have safe (or at least safer) APIs

I'm skeptical this could ever be made fully safe, however it will certainly be possible (and I'd say, encouraged) to build safe APIs on top of this.

The reason I'm skeptical basically comes down to the variety of different attributes a user could pass in and the fact that MS could add new attributes at any time. For instance, if MS added something which allowed the user to have some of the parent process's memory mapped into the child's address space, that would immediately preclude this from ever being truly safe.

My hope is that this extension method, it will enable authors of 3rd party libraries to make safe APIs via their own extension traits, even if those safe APIs never make it into the standard library.

[ChrisDenton]

Yes sorry, that last part was meant as a separate thought. I was meaning it would be possible to have a safe API for common operations such as a parent_handle or exploit_mitigation method. I agree that arbitrary ones would be impossible because we can't know what future attributes will be added and a lower-level method will always be useful for that.

[bjorn3]

Can you please use rebase instead of merge?

[Mark-Simulacrum]

r? @yaahc for T-libs-api review

I'm not familiar enough with Windows to review the impl either.

[TyPR124]

I will try to fix the git history sometime this week (or this weekend) and also I still need to open a tracking issue.

[TyPR124]

While considering what it might look like to make the method generic over the value type, I noticed that there is a difference in object safety between unix and windows CommandExt. Is this something to be concerned about? Windows is object safe, unix is not.

#[cfg(test)]
fn test_object_safety() {
    #[cfg(windows)]
    use std::os::windows::process::CommandExt;
    #[cfg(unix)]
    use std::os::unix::process::CommandExt;
    fn takes_dyn_command_ext(_: &dyn CommandExt) {}
    takes_dyn_command_ext(&std::process::Command::new(""));
}

Currently, if I were to add a generic method for Windows (without Self: Sized), it silently breaks object safety.

Looking at methods on the unix side, it appears to me the trait was originally object-safe in 1.0, but that was broken in 1.15 with before_exec, and then later pre_exec and arg0.

[TyPR124]

Can you please use rebase instead of merge?

I think this should be good now. Let me know if there is anything else I should do.

[TyPR124]

I've gone ahead and updated this to take ownership of a generic value type on each use. The T: Copy bound could be removed, but it seemed reasonable since Windows API will presumably need to make a copy of the value.

This also breaks object safety of the windows trait - I decided not to care about this since it is already broken for Unix and I don't think there is any practical use for dyn CommandExt over Command

[bors]

☔ The latest upstream changes (presumably #89174) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@TyPR124 you have conflicts to be resolved

[yaahc]

I agree with the idea of renaming to call it the raw attribute interface to specifically leave room for a safe interface in the future, even if we don't have to design that interface as part of this PR

[TyPR124]

I am having a very hard time with pulling most recent master... so just hoping it works to fix the conflicts through github. Not sure if the new commit is an issue.

[Kobzol]

Make sure that you don't have any pending changes in your workspace and then run this (assuming that you are on the branch of this PR):

$ git fetch origin master
$ git rebase origin/master

Then you need to fix conflicts, and run

$ git add -u
$ git rebase --continue
$ git push --force-with-lease

[TyPR124]

If my remotes look like this:

❯ git remote -v
origin  git@github.com:TyPR124/rust.git (fetch)
origin  git@github.com:TyPR124/rust.git (push)
upstream        https://github.com/rust-lang/rust (fetch)
upstream        https://github.com/rust-lang/rust (push)

Then should I replaced 'origin' with 'upstream' in those commands?

I was previously trying to follow https://rustc-dev-guide.rust-lang.org/git.html which to fix merge conflicts says "First, get a local copy of the conflicting changes: Checkout your local master branch with git checkout master, then git pull upstream master" but when I do that I just get pain. Does that info need updated?

Edit: I've done the above with upstream instead of origin. I think it looks okay now. Not sure if I've messed something up in the past, or if those instructions could be updated somehow. I do know in the past I've used github's website to update my forks master branch, maybe that is incompatible with Rust's workflow?

[Kobzol]

Yeah if you named the rustc repo upstream you should use upstream in the commands above.

Your master branch probably got out of sync with the upstream master. In that case just switch to master (git checkout master) and forcefully set it to the upstream master branch (git reset --hard upstream/master).

[Dylan-DPC]

@TyPR124 git pull does a merge (git pull is effectively fetch + merge)

next time just add the -r flag to it which will do a rebase instead of a merge (i.e. git pull -r <remote>/<branch>)

Docs on --rebase

[yaahc]

Edit: I've done the above with upstream instead of origin. I think it looks okay now. Not sure if I've messed something up in the past, or if those instructions could be updated somehow. I do know in the past I've used github's website to update my forks master branch, maybe that is incompatible with Rust's workflow?

Looks like you got it to me, I only see your two commits, no merge commits.

Regarding the rename, what do you think about renaming the method to process_raw_thread_attribute? Also, this is gonna need a tracking issue before we can merge it.

[yaahc]

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[TyPR124]

Sorry, it's been a while since I've been able to put much time into this.

I've found that the test I wrote is racy. I'm not sure exactly why yet. If I apply this PR to nightly-2021-08-18, the test passes every time I run it (though I'm not 100% convinced that it never fails on this version). At some point between 2021-11-18 and 2021-12-18, it seems like the issue was either introduced or made more likely to cause test failure.

A workaround that seems to be 100% effective in my testing is spawn the second (child) process with sleep 1. Without this, it seems that the child process dies before the wmic command runs, so wmic fails to find the process.

Re: renaming the method. I don't think process_raw_thread_attribute makes sense here. The name PROCESS_THREAD_ATTRIBUTE is, as far as I know, is becuase these are attributes that can be applied to either process' or threads. Since the context it is being used here is specifically for process spawning, I think just raw_attribute will be better - and I can't think of any reason why we'd need a [raw-]attribute() method other than this purpose.

[ChrisDenton]

it seems that the child process dies before the wmic command runs, so wmic fails to find the process

Could you add the wmic command to the child process itself? Edit: Hm, getting the current process id within cmd is harder than I thought. Maybe using powershell would be simpler? I.e.:

$(Get-Process -id $PID).Parent.Id

[yaahc]

Re: renaming the method. I don't think process_raw_thread_attribute makes sense here. The name PROCESS_THREAD_ATTRIBUTE is, as far as I know, is becuase these are attributes that can be applied to either process' or threads. Since the context it is being used here is specifically for process spawning, I think just raw_attribute will be better - and I can't think of any reason why we'd need a [raw-]attribute() method other than this purpose.

Sounds reasonable, 👍 for naming it to raw_attribute.

[TyPR124]

$(Get-Process -id $PID).Parent.Id

Good idea, but for me it only works in pwsh; not native powershell. I imagine that would be a problem.

wmic process where processid=$PID get parentprocessid seems to work in both, so I might give that a go.

[lzybkr]

$(Get-Process -id $PID).Parent.Id

Good idea, but for me it only works in pwsh; not native powershell. I imagine that would be a problem.

wmic process where processid=$PID get parentprocessid seems to work in both, so I might give that a go.

If you'd like a PowerShell option that works in powershell and pwsh - you can use:

(Get-CimInstance Win32_Process -Filter "ParentProcessId=$PID").ProcessId

This is still using WMI under the covers, just not invoking the wmic command line tool.

[EddieIvan01]

So, is there an expected plan for merging this PR?

[yaahc]

So, is there an expected plan for merging this PR?

I believe it's just waiting on the author to update the PR to rename the method and create a tracking issue.

[Dylan-DPC]

@TyPR124 any updates?

[JohnCSimon]

@TyPR124
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive