libstd: unix process spawning: fix bug with setting stdio #30490
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
| Stdio::Raw(fd@libc::STDERR_FILENO) => match cvt_r(|| libc::dup(fd)) { | ||
| Err(_) => fail(output), | ||
| Ok(fd) => { | ||
| let fd = RawStdio::new(fd); |
There was a problem hiding this comment.
Could this use FileDesc instead of RawStdio here? (as RawStdio is just a type alias)
|
Thanks! Could you be sure to also add a test for this? |
|
@alexcrichton addressed all comments! I wasn't sure about the right place for such a test, but I think run-pass is a good fit. |
|
|
||
| { | ||
| let stdout = File::create(&stdout_file_path).expect("failed to create stdout file"); | ||
| let stderr = File::create(&stderr_file_path).expect("failed to create stderr file"); |
There was a problem hiding this comment.
Could this test be written without creating files? Keeping everything in-memory is generally better for isolation. I think this should basically just be able to use combinations of pipes, right?
|
@alexcrichton rewrote the test to use pipes instead of files and to re-exec itself instead of calling a shell |
|
@bors: r+ 751fce0974dd4f634539c37ba759d13c4f894d09 Thanks! |
|
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/2525/steps/test/logs/stdio |
bors
added a commit
that referenced
this pull request
Dec 25, 2015
* If the requested descriptors to inherit are stdio descriptors there
are situations where they will not be set correctly
* Example: parent's stdout --> child's stderr
parent's stderr --> child's stdout
* Solution: if the requested descriptors for the child are stdio
descriptors, `dup` them before overwriting the child's stdio
|
Doh! Forgot some @alexcrichton @Manishearth should be fixed now |
|
⌛ Testing commit 7f7a059 with merge 6f86431... |
|
💔 Test failed - auto-win-msvc-32-opt |
|
@bors: retry On Mon, Jan 11, 2016 at 3:21 PM, bors notifications@github.com wrote:
|
bors
added a commit
that referenced
this pull request
Jan 11, 2016
* If the requested descriptors to inherit are stdio descriptors there
are situations where they will not be set correctly
* Example: parent's stdout --> child's stderr
parent's stderr --> child's stdout
* Solution: if the requested descriptors for the child are stdio
descriptors, `dup` them before overwriting the child's stdio
Example of a program which exhibits the bug:
```rust
// stdio.rs
use std::io::Write;
use std::io::{stdout, stderr};
use std::process::{Command, Stdio};
use std::os::unix::io::FromRawFd;
fn main() {
stdout().write_all("parent stdout\n".as_bytes()).unwrap();
stderr().write_all("parent stderr\n".as_bytes()).unwrap();
Command::new("sh")
.arg("-c")
.arg("echo 'child stdout'; echo 'child stderr' 1>&2")
.stdin(Stdio::inherit())
.stdout(unsafe { FromRawFd::from_raw_fd(2) })
.stderr(unsafe { FromRawFd::from_raw_fd(1) })
.status()
.unwrap_or_else(|e| { panic!("failed to execute process: {}", e) });
}
```
Before:
```
$ rustc --version
rustc 1.7.0-nightly (8ad12c3 2015-12-19)
$ rustc stdio.rs && ./stdio >out 2>err
$ cat out
parent stdout
$ cat err
parent stderr
child stdout
child stderr
```
After (expected):
```
$ rustc --version
rustc 1.7.0-dev (712ecce 2015-12-19)
$ rustc stdio.rs && ./stdio >out 2>err
$ cat out
parent stdout
child stderr
$ cat err
parent stderr
child stdout
```
|
I think the original report here shows an program which has undefined behaviour. The documentation for
Calling this on 0, 1 or 2 is clearly wrong. |
This was referenced Nov 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
are situations where they will not be set correctly
parent's stderr --> child's stdout
descriptors,
dupthem before overwriting the child's stdioExample of a program which exhibits the bug:
Before:
After (expected):