Skip to content
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

Parent process with "windows" subsystem can't properly create console child process #101645

Closed
CrendKing opened this issue Sep 10, 2022 · 7 comments · Fixed by #103459
Closed
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@CrendKing
Copy link

CrendKing commented Sep 10, 2022

I tried this code:

App P:

#![windows_subsystem = "windows"]

fn main() {
    std::process::Command::new("<path_to_app_C>").spawn().unwrap();
    std::thread::sleep(std::time::Duration::from_secs(100));
}

App C:

fn main() {
    loop {
        println!("Hello World!");
        std::thread::sleep(std::time::Duration::from_secs(1));
    }
}

Build this in Windows.

I expected to see this happen:

Since P has "windows" subsystem, it will create no console window (i.e. no conhost.exe process). Since C is a console app, when the process is created, I expect to see one console window that prints "Hello World" infinitely.

Instead, this happened: explanation

App C creates its console window but no text is printed.

What I suspect the cause of the issue

In Windows, spawn() eventually calls to CreateProcessW with a STARTUPINFO, whose dwFlags is hard coded to STARTF_USESTDHANDLES. Additionally, it also get device handles of stdin, stdout and stderr with GetStdHandle, and set them to the STARTUPINFO. Since app P uses "windows" subsystem, it does not have console. stdout is always INVALID_HANDLE_VALUE. Combining STARTF_USESTDHANDLES with INVALID_HANDLE_VALUE in STARTUPINFO causes the unexpected behavior.

If I make a call to CreateProcessW with same parameters except a "zeroed" STARTUPINFO, it exhibits the correct behavior.

Suggestion

spawn() should only conditionally set the STARTF_USESTDHANDLES flag if it can obtain those device handles, and never set any of those handles INVALID_HANDLE_VALUE. Like this (pseudo code):

let mut si = zeroed_startupinfo();
si.cb = mem::size_of::<c::STARTUPINFO>() as c::DWORD;

si.hStdInput = stdin.to_handle().unwrap_or(0);
si.hStdOutput = stdout.to_handle().unwrap_or(0);
si.hStdError = stderr.to_handle().unwrap_or(0);

if si.hStdInput != 0 || si.hStdOutput != 0 || si.hStdError != 0 {
    si.dwFlags |= c::STARTF_USESTDHANDLES;
}

Meta

rustc --version --verbose:

rustc 1.63.0 (4b91a6ea7 2022-08-08)
binary: rustc
commit-hash: 4b91a6ea7258a947e59c6522cd5898e7c0a6a88f
commit-date: 2022-08-08
host: x86_64-pc-windows-msvc
release: 1.63.0
LLVM version: 14.0.5
@CrendKing CrendKing added the C-bug Category: This is a bug. label Sep 10, 2022
@CrendKing CrendKing changed the title Parent process with "windows" subsystem can't proper create console child process Parent process with "windows" subsystem can't properly create console child process Sep 10, 2022
@ChrisDenton ChrisDenton added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 10, 2022
@ChrisDenton
Copy link
Member

I think this will need a T-libs-api decision as the current documentation for Command::stdout (and stdin/stderr) is:

Defaults to inherit when used with spawn or status, and defaults to piped when used with output.

I would guess that this "inherit by default" is why it's implemented the way it is. But perhaps there's some wiggle room with what it means to inherit handles that don't exist so we can change it. In either case it would be good to have some way to use platform default behaviour here.

@rustbot label -T-libs +T-libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 10, 2022
@CrendKing
Copy link
Author

CrendKing commented Sep 10, 2022

what it means to inherit handles that don't exist

That's exactly my point. If there is nothing to inherit in the first place, just inherit nothing. The current behavior (INVALID_HANDLE_VALUE) is simply wrong. I believe fixing it doesn't violate the current specification.

@CrendKing
Copy link
Author

Basically, currently there are only two options when handling the stdout/stderr: inherit or pipe. I think there should be a third option: isolated - make the child handle its console by itself. For example, if user calls .stdout(std::process::Stdio::null()) and .stdout(std::process::Stdio::null()), he basically says "the parent process doesn't care about the child's output". It is the same as the "windows" subsystem case above. In this case, the STARTF_USESTDHANDLES flag should never be set, regardless if spawn() or output() is called. More importantly, this is de facto the default way of how a process is created in Windows. Defaulting to STARTF_USESTDHANDLES really makes me confused before reading the internal source code.

@kryptan
Copy link
Contributor

kryptan commented Nov 20, 2022

Since app P uses "windows" subsystem, it does not have console. stdout is always INVALID_HANDLE_VALUE.

Not always. Parent process can still set those handles to something other than INVALID_HANDLE_VALUE. For example, if you launch App P from IntelliJ it actually prints "Hello World!" in a loop but not in the spawned console which is empty, but in IntelliJ internal terminal. GetStdHandle(STD_OUTPUT_HANDLE) returns non invalid handle.

if si.hStdInput != 0 || si.hStdOutput != 0 || si.hStdError != 0 {
    si.dwFlags |= c::STARTF_USESTDHANDLES;
}

This solution would make behaviour different depending on how you launch your app.

Also, you can explicitly ask to create new console with creation_flags(CREATE_NEW_CONSOLE.0) even if your app already uses console subsystem. In this case you also likely want to avoid inheriting handles because you want output of the spawned process to be displayed in the new console, not in the existing one. This solution doesn't help with this.

@CrendKing
Copy link
Author

from IntelliJ

I can't verify your statements since I don't have IntelliJ atm, but even it's true, isn't it IntelliJ setting up the console with maybe some special debugging API? How to you modify the code of P to achieve the same? And realistically speaking, do people actually do that with GUI apps?

want to avoid inheriting handles

I think in this case, it's impossible to be 100% sure where the user wants the output at. New option might be the way to go.

@workingjubilee
Copy link
Member

Also, you can explicitly ask to create new console with creation_flags(CREATE_NEW_CONSOLE.0) even if your app already uses console subsystem. In this case you also likely want to avoid inheriting handles because you want output of the spawned process to be displayed in the new console, not in the existing one. This solution doesn't help with this.

@kryptan Your prediction was dead-on, as it came true in #105203. While we can do this, as #103459 is proposing, it seems best here that we create a function, or set of functions, that explicitly handle this kind of situation rather than guessing.

@ChrisDenton
Copy link
Member

ChrisDenton commented Dec 5, 2022

Even with new APIs, we still need reasonable default behaviour. And the Command API is documented as inheriting handles by default. However, currently the API neglects the "no handle set" case entirely as that's not really a thing in POSIX or ISO C (well, you can close stdio but that's usually considered a Bad Thing unless you're being very very careful).

@bors bors closed this as completed in 01fbc5a Dec 7, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
Pass on null handle values to child process

Fixes rust-lang#101645

In Windows, stdio handles are (semantically speaking) `Option<Handle>` where `Handle` is a non-zero value. When spawning a process with `Stdio::Inherit`, Rust currently turns zero values into `-1` values. This has the unfortunate effect of breaking console subprocesses (which typically need stdio) that are spawned from gui applications (that lack stdio by default) because the console process won't be assigned handles from the newly created console (as they usually would in that situation). Worse, `-1` is actually [a valid handle](https://doc.rust-lang.org/std/os/windows/io/struct.OwnedHandle.html) which means "the current process". So if a console process, for example, waits on stdin and it has a `-1` value then the process will end up waiting on itself.

This PR fixes it by propagating the nulls instead of converting them to `-1`.

While I think the current behaviour is a mistake, changing it (however justified) is an API change so I think this PR should at least have some input from t-libs-api. So choosing at random...

r? `@joshtriplett`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants