Pass on null handle values to child process

[ChrisDenton]

Fixes #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 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

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[ChrisDenton]

r? @thomcc I think this can be considered a bug fix (see also rust-lang/libs-team#137 (comment)) so this is ready for review.

[workingjubilee]

It's too bad we can't run Crater on Windows.

I am in favor of this change or something like it as a bug fix but I should note that I feel we still have an uncovered API use-case in situations like #105203 so the underlying issue of wanting a clearer API saying something like "please make something new here" has not been resolved.

Also I should note that previously we removed some code that also did this conversion: 7ddf41c

I feel like we should have some kind of test here.

[kryptan]

Does this PR rely on some undocumented behaviour? Windows documentation doesn't seem to say what happens when you set STARTF_USESTDHANDLES but then pass NULLs as std handles.

GUI apps can have stdio handles. Inheriting them while still creating a new useless empty console window doesn't look like a sane default behaviour but it is what happens with or without this PR.

[workingjubilee]

That's kind of why I linked that commit... it documented what can happen when these values are NULL. Apparently the answer is Unfun.

[ChrisDenton]

Also I should note that previously we removed some code that also did this conversion: 7ddf41c

That commit only removed a no-op. The code already turned nulls to INVALID_HANDLE_VALUE at an earlier point (as noted in the commit description) so removing it had not effect on behaviour. This PR changes that earlier code so that it doesn't set INVALID_HANDLE VALUE (aka -1, which is in fact a valid handle value).

Does this PR rely on some undocumented behaviour? Windows documentation doesn't seem to say what happens when you set STARTF_USESTDHANDLES but then pass NULLs as std handles.

I'm uncertain if this is documented somewhere but in any case this PR does not actually rely on any particular behaviour. A null handle is an invalid handle value (unlike INVALID_HANDLE_VALUE which is a valid handle value) so its options are to either pass on null handles unchanged or replace them with console handles. Either behaviour works for us, or at least works better than the current behaviour.

GUI apps can have stdio handles. Inheriting them while still creating a new useless empty console window doesn't look like a sane default behaviour but it is what happens with or without this PR.

A console process can be spawned detached if you do not want a console to actually be created. Even if a console is created while inheriting handles, it's not necessarily useless. It can still be read from a written to. But, as you say, this PR isn't changing that behaviour.

[ChrisDenton]

Ah, documentation is in the GetStdHandle docs:

When attaching to a new console, standard handles are always replaced with console handles unless STARTF_USESTDHANDLES was specified during process creation.

If the existing value of the standard handle is NULL, or the existing value of the standard handle looks like a console pseudohandle, the handle is replaced with a console handle.

When a parent uses both CREATE_NEW_CONSOLE and STARTF_USESTDHANDLES to create a console process, standard handles will not be replaced unless the existing value of the standard handle is NULL or a console pseudohandle.

[ChrisDenton]

As per #105203, I've added another commit that avoids setting STARTF_USESTDHANDLES in the case where all handles are unset. This should help reduce differences between Windows 7 and Windows 8+ with how they replace unset handles.

[thomcc]

Super sorry for the delay here (busy and didn't want to half-ass reviews). This looks good to me.

@bors r+

[bors]

📌 Commit 93b774a has been approved by thomcc

It is now in the queue for this repository.

[ChrisDenton]

No worries! I'm well aware people have lives outside of Rust and a week is not long to wait at all. 😊

[bors]

⌛ Testing commit 93b774a with merge 01fbc5a...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 01fbc5a to master...

[rust-timer]

Finished benchmarking commit (01fbc5a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.