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

std::process::Command: piped stderr hangs in Windows #38811

Closed
dherman opened this issue Jan 3, 2017 · 4 comments · Fixed by #38835
Closed

std::process::Command: piped stderr hangs in Windows #38811

dherman opened this issue Jan 3, 2017 · 4 comments · Fixed by #38835
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dherman
Copy link
Contributor

dherman commented Jan 3, 2017

It looks like the use of overlapped I/O on stdio ports may cause some problems in Windows. When I try to shell out to npm install using Stdio::piped() on the stderr port, the command runs successfully but hangs at the end.

@alexcrichton pointed me to this commit as the likely culprit: 7c3038f

Here's the smallest test case I could figure out how to create: https://github.com/dherman/hang-command

I wanted to try to come up with a minimal Node script to shell out to, as opposed to npm install. Unfortunately, I'm not good enough at debugging npm and Node to figure out exactly what they're doing that causes the hang.

@alexcrichton
Copy link
Member

Ok I've tested this and it's a stable-to-stable regression that happened between 1.8 and 1.9. I believe the 1.9 release is indeed the first one with #31618, the likely change which is a culprit. Tagging appropriately as such.

@alexcrichton alexcrichton added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-nominated labels Jan 4, 2017
@rprichard
Copy link
Contributor

It looks like Rust is passing overlapped-mode pipe handles to the child process. The pipe::anon_pipe function returns two overlapped HANDLES. I'm not sure the overlapped mode can be changed after the fact -- maybe ReOpenFile would work -- but I don't see it used anywhere, and I'd wonder about its use on the server handle.

If the child inherits overlapped-mode handles, it would need to do async I/O to stdin/stdout/stderr. By convention, it does sync I/O using lpOverlapped==NULL, but that has undefined behavior if the handles are overlapped pipes.

https://blogs.msdn.microsoft.com/oldnewthing/20120411-00/?p=7883
https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

FWIW: the server and client ends of a named pipe aren't quite symmetric, but I'm guessing it doesn't matter whether the child process receives a server pipe or a client pipe? The interesting thing is that writing to a broken pipe returns ERROR_NO_DATA, but reading from one returns ERROR_BROKEN_PIPE. Apparently this is true for both ends. (I apparently wrote an extensive test program for this back in June'16.) I'd probably pass the client end to the child process, because it doesn't make sense that a program can call DisconnectNamedPipe on its stdio handles. (DisconnectNamedPipe doesn't work on client handles.)

I made a Rust + C++ test case demonstrating how passing overlapped handles to the child breaks:
https://gist.github.com/rprichard/37bfd1dd6a2ef91ba7af49efbe0461ed

@alexcrichton
Copy link
Member

Fascinating! Thanks for the links @rprichard! That perfectly explains I think what's going on here.

I think I can see the fix here, so I'll work on that pronto.

@rprichard
Copy link
Contributor

Yeah, I think the fix is easy -- use non-overlapped handles for the child but overlapped handles for the parent.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 4, 2017
This commit fixes a mistake introduced in rust-lang#31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes rust-lang#38811
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this issue Jan 6, 2017
This commit fixes a mistake introduced in rust-lang#31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes rust-lang#38811
bors added a commit that referenced this issue Jan 6, 2017
std: Don't pass overlapped handles to processes

This commit fixes a mistake introduced in #31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes #38811
dherman pushed a commit to neon-bindings/neon that referenced this issue Feb 18, 2017
… in Windows, eliminate the workaround hack for the rust issue that's been fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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.

3 participants