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.

Sign up for GitHub

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

Jump to bottom

std: Fix inheriting standard handles on windows #24873

Merged
merged 1 commit into from Apr 30, 2015
Merged

std: Fix inheriting standard handles on windows #24873

merged 1 commit into from Apr 30, 2015

Conversation

alexcrichton
Copy link
Member

Currently if a standard I/O handle is set to inherited on Windows, no action is
taken and the slot in the process information description is set to
INVALID_HANDLE_VALUE. Due to our passing of STARTF_USESTDHANDLES, however,
this means that the handle is actually set to nothing and if a child tries to
print it will generate an error.

This commit fixes this behavior by explicitly creating stdio handles to be
placed in these slots by duplicating the current process's I/O handles. This is
presumably what previously happened silently by using a file-descriptor-based
implementation instead of a HANDLE-centric implementation.

Along the way this cleans up a lot of code in Process::spawn for Windows by
ensuring destructors are always run, using more RAII, and limiting the scope of
unsafe wherever possible.

@rust-highfive
Copy link
Collaborator

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @aturon

Also nominating for beta cherry-pick.

@rust-highfive rust-highfive assigned aturon and unassigned pcwalton Apr 27, 2015
@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 27, 2015
@alexcrichton alexcrichton mentioned this pull request Apr 27, 2015
Merged
tamird
tamird reviewed Apr 27, 2015
View reviewed changes
src/libstd/sys/windows/process2.rs

let mut pi = zeroed_process_information();
let mut create_err = None;
let stdio_handle = |io: &Stdio, id: libc::DWORD| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be an inner function rather than a closure?

by the way, what are the implications of using a closure (which doesn't close over anything) over an inner function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow I guess this doesn't close over anything any more (it did previously). If you're not closing over anything then I'd recommend a free function personally as you probably need to have type annotations anyway.

@alexcrichton alexcrichton force-pushed the fix-windows-stdio branch 3 times, most recently from 7694ee7 to 1d8388e Compare April 27, 2015 22:00
tamird
tamird reviewed Apr 27, 2015
View reviewed changes
src/libstd/sys/windows/process2.rs
create_err = Some(Error::last_os_error());
}
})
let (envp, _data) = make_envp(cfg.env.as_ref());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why return _data here?

@aturon
Copy link
Member

aturon commented Apr 27, 2015

Oy, fun bug :)

This PR LGTM. Cute use of "RAII".

@aturon
Copy link
Member

aturon commented Apr 27, 2015

@tamird The reason to return _data is that it's holding ownership of the actual data that the raw pointer is pointing to (in the case that it's not null). The data itself is only being used via the pointer (which is passed down to C), but you want to keep the vector alive for the duration of the function to keep that data allocated. This is a somewhat subtle use of an RAII-ish pattern.

@alexcrichton
Copy link
Member Author

@bors: r=aturon cb41b82

@tamird
Copy link
Contributor

tamird commented Apr 27, 2015

@aturon cool, that's what I suspected, thanks. Allocating an empty vector in the None case strikes me as odd though; is there an owning pointer type that can be used to express this pattern without the extra allocation?

@alexcrichton
Copy link
Member Author

It's possible that it could just return a Vec<T> and then .as_ptr() could be used as necessary, but this is just mirroring what unix does for now.

@alexcrichton alexcrichton mentioned this pull request Apr 28, 2015
Closed
@bors
Copy link
Contributor

bors commented Apr 28, 2015

⌛ Testing commit cb41b82 with merge f20b8e8...

@bors
Copy link
Contributor

bors commented Apr 28, 2015

💔 Test failed - auto-win-32-opt

@alexcrichton
Copy link
Member Author

@bors: retry

On Tue, Apr 28, 2015 at 1:43 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-32-opt
http://buildbot.rust-lang.org/builders/auto-win-32-opt/builds/4389

—
Reply to this email directly or view it on GitHub
#24873 (comment).

@bors
Copy link
Contributor

bors commented Apr 29, 2015

⌛ Testing commit cb41b82 with merge 99f628b...

@bors
Copy link
Contributor

bors commented Apr 29, 2015

💔 Test failed - auto-win-64-nopt-t

@alexcrichton
std: Fix inheriting standard handles on windows
c1149ed 
Currently if a standard I/O handle is set to inherited on Windows, no action is
taken and the slot in the process information description is set to
`INVALID_HANDLE_VALUE`. Due to our passing of `STARTF_USESTDHANDLES`, however,
this means that the handle is actually set to nothing and if a child tries to
print it will generate an error.

This commit fixes this behavior by explicitly creating stdio handles to be
placed in these slots by duplicating the current process's I/O handles. This is
presumably what previously happened silently by using a file-descriptor-based
implementation instead of a `HANDLE`-centric implementation.

Along the way this cleans up a lot of code in `Process::spawn` for Windows by
ensuring destructors are always run, using more RAII, and limiting the scope of
`unsafe` wherever possible.
@alexcrichton
Copy link
Member Author

@bors: r=aturon c1149ed

@alexcrichton alexcrichton mentioned this pull request Apr 29, 2015
Closed
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 29, 2015
@alexcrichton
rollup merge of rust-lang#24873: alexcrichton/fix-windows-stdio
cc04cd4 
Conflicts:
	src/libstd/sys/windows/fs2.rs
@bors bors merged commit c1149ed into rust-lang:master Apr 30, 2015
@alexcrichton alexcrichton deleted the fix-windows-stdio branch April 30, 2015 02:05
@pnkfelix
Copy link
Member

triage: beta-accepted

(as a special case for the 1.0 release; post 1.0 changes like this probably would not be accepted for backport)

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 30, 2015
@alexcrichton alexcrichton mentioned this pull request Apr 30, 2015
Merged
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 30, 2015
@brson
Copy link
Contributor

brson commented Apr 30, 2015

Backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aturon aturon

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@alexcrichton @rust-highfive @aturon @tamird @bors @pnkfelix @brson @pcwalton