Add new inherit_handles flag to CommandExt trait

[michaelvanstraten]

This PR adds a new flag to the CommandExt trait to set whether to inherit the handles of the calling process ([ref][1]).

This is necessary when, for example, spawning a process with a pseudoconsole attached.

r? @ChrisDenton

ACP: rust-lang/libs-team#264
[1]: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw

[michaelvanstraten]

Should this have a stabilization issue attached to it?

[the8472]

First you should create an API change proposal to see if libs-api wants it.

[michaelvanstraten]

I recently changed the exact same interface and didn't need an ACP, is this really required here?

[the8472]

No, not required

Note that an ACP is not strictly required: you can just go ahead and submit a pull request with an implementation of your proposed API, with the risk of wasted effort if the library team ends up rejecting this feature. However do note that this risk is always present even if an ACP is accepted, as the library team can end up rejecting a feature in the later parts of the stabilization process.

[the8472]

Anyway, does windows have an equivalent to posix_spawn_file_actions_adddup2? I.e. a way to provide a list of descriptors that should be explicitly passed to the child process rather than relying on inheritance flags on handles?

Inheritance is questionable in multi-threaded programs because different command-spawning actions may want to pass different files to the child.

Also, how does one communicate to a child that a specific handle is available? On unix we've run into issues defining IO safety when passing ownership through the environment. Are there better ways to send handles to another process?

This is necessary when, for example, spawning a process with a pseudoconsole attached.

Alternatively, can we limit this to passing a set of well-defined handles?

[ChrisDenton]

This is necessary when, for example, spawning a process with a pseudoconsole attached.

It is not necessary when creating a pseudoconsole to disable handle inheritance. Doing so does however allow us to skip the mutex we currently use.

Anyway, does windows have an equivalent to posix_spawn_file_actions_adddup2? I.e. a way to provide a list of descriptors that should be explicitly passed to the child process rather than relying on inheritance flags on handles?

Yes. You need to set inherit_handles to true and then set the PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute. Rust should have been doing this from the start, imho, but not sure if we can change it now.

I recently changed the exact same interface and didn't need an ACP, is this really required here?

The reason the previous API change didn't require an ACP was because the change was already accepted under an older system (and I asked privately if this was still OK). A new API would need an ACP.

[michaelvanstraten]

It is not necessary when creating a pseudoconsole to disable handle inheritance. Doing so does however allow us to skip the mutex we currently use.

Do you know why all of them set bInheritHandles to false? I have look at a dozenth reference implementation so far and all do so.

[michaelvanstraten]

Okay I just tested it, indeed not necessary to set bInheritHandles.

Would this still be a useful extension to the CommandExt trait?

[ChrisDenton]

Do you know why all of them set bInheritHandles to false? I have look at a dozenth reference implementation so far and all do so.

The most likely the simplest explanation is "because the example code does". But there are good reasons to disable handle inheritance when it's not needed: inheriting unnecessary handles is pointless and also prevents kernel objects from being cleaned up. In the worst case it may also allow the child process to mess with the handles it inherits, though it would have to find them first.

Would this still be a useful extension to the CommandExt trait?

I think it would for the above reasons.

[michaelvanstraten]

I don't know if windows has the concept of confidential handles but preventing the child from acquiring those handles could improve security of spawning commands.

[michaelvanstraten]

Okay, just found the following list in the windows documentation:

Processes can inherit or duplicate handles to the following types of objects:

• Access Token
• Communications device
• Console input
• Console screen buffer
• Desktop
• Directory
• Event
• File
• File mapping
• Job
• Mailslot
• Mutex
• Pipe
• Process
• Registry key
• Semaphore
• Socket
• Thread
• Timer

[michaelvanstraten]

rust-lang/libs-team#264

[michaelvanstraten]

@ChrisDenton should I message somebody about this in rust internals?

[ChrisDenton]

You don't need to do anything other than be patient 🙂. ACPs are discussed in a weekly meeting (time permitting).

[michaelvanstraten]

ACPs are discussed in a weekly meeting (time permitting).

Sorry, didn't know that.

[bors]

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

[pixmaip]

Hi everyone,
I am unearthing this discussion as this proposed API can be mandatory in some cases.

While handle inheritance can be selectively configured using the PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute, in the case of protected processes, the bInheritHandles parameter still needs to be set to FALSE in all cases, otherwise the CreateProcess call will fail.

This effectively means that Rust programs running as PPL currently cannot spawn non-protected processes using the standard library, as the spawn will always fail, as deccribed in the Microsoft documentation of CreateProcess:

Protected Process Light (PPL) processes: The generic handle inheritance is blocked when a PPL process creates a non-PPL process since PROCESS_DUP_HANDLE is not allowed from a non-PPL process to a PPL process. See Process Security and Access Rights

[michaelvanstraten]

I have an exam on Monday but after that I should be able to work on this.

[michaelvanstraten]

@pixmaip would the following suggestion aid your need?
rust-lang/libs-team#264 (comment)

[pixmaip]

No, the suggestion you mentioned might be good for most usecases, but when running processes as PPL, passing a list of handles to inherit cannot be done.
For the CreateProcess API to succeed, you need to have bInheritHandles set to FALSE.

This is because PPL processes are inherently prevented by the system from leaking handles to non-protected processes, so Windows does not allow it.

Note that setting bInheritHandles to FALSE will automatically prevent standard handles to work properly, meaning that stdin will not be passed through to the child process, and stdout/err will not output any data. The only possible "interaction" with the child process is through its exit status.

[michaelvanstraten]

Okay, then this should mergable independently of #123604. Please also raise your issue with the proposed solution over at rust-lang/libs-team#264 so it can be re-discussed by the lib team or maybe @ChrisDenton you can note it.

[PaulDance]

As per rust-lang/libs-team#264 (comment), the ACP about this seems to stand firm on having only a single API that sets a specific set of handles to inherit, with the special case of an empty slice not doing anything about that and setting bInheritHandles to FALSE. I guess the documentation would be the main clarification of this distinction in behavior.

@michaelvanstraten do you therefore think the current PR is salvageable for this in any way? If so, do you think you would have bandwidth these days to do so?

The setup code for using PROC_THREAD_ATTRIBUTE_HANDLE_LIST is apparently not trivial, but maybe with #123604 already being merged, it would be more easy to achieve. In any case, if you don't have time, @pixmaip and I could try to make a new PR for this.

[michaelvanstraten]

As per rust-lang/libs-team#264 (comment), the ACP about this seems to stand firm on having only a single API that sets a specific set of handles to inherit, with the special case of an empty slice not doing anything about that and setting bInheritHandles to FALSE. I guess the documentation would be the main clarification of this distinction in behavior.

Could be a bit tricky and confusing for users, as I mentioned in rust-lang/libs-team#264 (comment).

@michaelvanstraten do you therefore think the current PR is salvageable for this in any way? If so, do you think you would have bandwidth these days to do so?

I currently have a deadline at work, which doesn't leave me with much time. I also have exams from the beginning of February to roughly the 20th, after then it should not be an issue. I hope I can work with you on it a bit in the meantime.

[PaulDance]

As per rust-lang/libs-team#264 (comment), this should now be possible again and pretty much as is since the part about a list of handles can be done separately. @michaelvanstraten a rebase would be needed to resolve the conflicts, please.

@rustbot label -S-waiting-on-ACP

[michaelvanstraten]

@PaulDance, i'll get on it.

[michaelvanstraten]

@PaulDance would you take over the FCP to stabilize this?

[PaulDance]

Alright, I'll start on this some time tomorrow.

[michaelvanstraten]

Okay, I think this needs to land first. But we should probably create a tracking issue using the template.

[PaulDance]

Indeed. By "start on this", I meant first documenting myself on the involved processes that I didn't know well and then opening the tracking issue. So yes, I can push the stabilization forward when the time comes.

In the meantime, I've now opened #146407 for this, so you should be able to use it here. Some of the feature implementation steps may still need to be done, but I don't know know if they all apply to the standard library as well.

I have an idea for some basic tests for this: spawning C:\Windows\System32\cmd.exe /C "echo something" with the flag set or not, as in one case the output should be received and in the other it should not. I'll attempt this later today and post the results further down. Any other idea is welcome, of course.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[michaelvanstraten]

@PaulDance, maybe you could add a test, I don't have a Windows machine.

[PaulDance]

@michaelvanstraten The following works well for me and should cover the feature adequately I think:

#[cfg(test)]
mod tests {
    use std::os::windows::process::CommandExt;
    use std::process::Command;

    #[test]
    fn test_windows_commandext_inherit_handles_true_stdout() {
        let out = Command::new(r"C:\Windows\System32\cmd.exe")
            .args(["/C", "echo Hello from StdOut!"])
            .inherit_handles(true)
            .output()
            .unwrap();
        dbg!(&out);

        // Here, it is as if `inherit_handles` was not used because it defaults
        // to `true`, so the following is very standard and not so surprising.
        assert!(out.status.success());
        assert!(out.stderr.is_empty());
        assert_eq!(
            str::from_utf8(&out.stdout).unwrap(),
            "Hello from StdOut!\r\n"
        );
    }

    #[test]
    fn test_windows_commandext_inherit_handles_true_stderr() {
        let out = Command::new(r"C:\Windows\System32\cmd.exe")
            .args(["/C", "echo Hello from StdErr!>&2"])
            .inherit_handles(true)
            .output()
            .unwrap();
        dbg!(&out);

        // Here, it is as if `inherit_handles` was not used because it defaults
        // to `true`, so the following is very standard and not so surprising.
        assert!(out.status.success());
        assert!(out.stdout.is_empty());
        assert_eq!(
            str::from_utf8(&out.stderr).unwrap(),
            "Hello from StdErr!\r\n"
        );
    }

    #[test]
    fn test_windows_commandext_inherit_handles_false_stdout() {
        let out = Command::new(r"C:\Windows\System32\cmd.exe")
            .args(["/C", "echo Hello from StdOut!"])
            .inherit_handles(false)
            .output()
            .unwrap();
        dbg!(&out);

        // Contrary to the above tests, this now fails as the handles are
        // simply not available in the process, so no write can be performed.
        assert!(!out.status.success());
        assert_eq!(out.status.code().unwrap(), 1);
        // Both standard output and error streams therefore end up empty.
        assert!(out.stdout.is_empty());
        assert!(out.stderr.is_empty());
    }

    #[test]
    fn test_windows_commandext_inherit_handles_false_stderr() {
        let out = Command::new(r"C:\Windows\System32\cmd.exe")
            .args(["/C", "echo Hello from StdErr!>&2"])
            .inherit_handles(false)
            .output()
            .unwrap();
        dbg!(&out);

        // Contrary to the above tests, this now fails as the handles are
        // simply not available in the process, so no write can be performed.
        assert!(!out.status.success());
        assert_eq!(out.status.code().unwrap(), 1);
        // Both standard output and error streams therefore end up empty.
        assert!(out.stdout.is_empty());
        assert!(out.stderr.is_empty());
    }
}

This is using a private build that already includes the forcibly-enabled feature, so is done externally to the stdlib, hence the use std::s. It should be easy to adapt to the stdlib tests, however, as there isn't a lot of adherence to much.

I guess you should be able to throw this at the CI and see what happens. Otherwise, I was simply using a local VM and it worked just fine. The bothersome part is more the cross compilation, of course.

[michaelvanstraten]

Looks good, I will try to integrate it later today.

An alternative would be to see which handles where inherited using winapi. This could then also be used by the other new feature to pass a list of handles.

[PaulDance]

An alternative would be to see which handles where inherited using winapi.

Indeed, although it would be a bit more involved: I think it would either require a separate program compiled just for the occasion and that does this, but I don't know if this something usually acceptable, or some kind of PowerShell dark magic I don't yet know of in order to rely on already-existing programs, like what I proposed.

[michaelvanstraten]

I think it would either require a separate program compiled just for the occasion and that does this, but I don't know if this something usually acceptable, or some kind of PowerShell dark magic I don't yet know of in order to rely on already-existing programs, like what I proposed.

I don't know if the inherited handles count towards the "open handles" but if so GetProcessHandleCount would sound like something we could use. I would take over the implementation if this sounds right.

[PaulDance]

GetProcessHandleCount would sound like something we could use

Yes, it does, so should be tried! I guess it could for example be added to the tests I proposed as additional assertion steps of each case, but since an OpenProcess or equivalent would be needed in order to get a handle to the process before calling GetProcessHandleCount on it, it might require starting it suspended to ensure it lives long enough, or just using the pause command or something like that if still using cmd.exe as the child process.

[PaulDance]

I've tried this a bit, and I'm not sure it is worth the effort:

• the count is relatively chaotic for cmd.exe: I've seen it range from 10 to 21;
• I couldn't observe much of a difference between the cases of having bInheritHandles to TRUE or FALSE, which could indicate that the standard streams somehow don't count into it;
• the setup code gets kind of ugly and ends up with uncontrollably-fixed delays;

[michaelvanstraten]

So there are, "ui" tests that would allow us to do this in a bit easier without having to rely on the cmd executable.

I will try to hack something up with what you already provided, I sadly can't cross compile or test on a VM since I'm in a camper on the road.

[michaelvanstraten]

@PaulDance, could you check if this works? I don't want to abuse CI for this (should I?).

I would agree that the test you initially provided was more robust and would even work for the feature where you pass a list of handles to inherit.

If using the GetProcessHandleCount doesn't work, I would just integrate your initial suggestion with the UI tests.

win-inherit-handles-test.patch

[PaulDance]

Ah yes, this should work indeed, thanks. I currently don't have access to my work setup, so my testing would need to wait for more than a week. In the meantime however, I would say using the CI for what it is should be acceptable, as long as it's not excessive I guess.

[michaelvanstraten]

That didn't actually seam to run any tests

[PaulDance]

I'm really not an expert in this, but maybe try using @bors try jobs=<job1,job2,...>? The docs mention special permissions though, so it might not work. That, or going through the cross-compilation and VM setup, or just waiting for my (epic) return from vacation.