Skip to content

Commit

Permalink
libnative: process spawning should not close inherited file descriptors
Browse files Browse the repository at this point in the history
* The caller should be responsible for cleaning up file descriptors
* If a caller safely creates a file descriptor (via
  native::io::file::open) the returned structure (FileDesc) will try to
  clean up the file, failing in the process and writing error messages
  to the screen.
* This should not happen as the caller has no public interface for
  telling the FileDesc structure to NOT free the underlying fd.
* Alternatively, if another file is opened under the same fd held by
  the FileDesc structure returned by native::io::file::open, it will
  close the wrong file upon destruction.
  • Loading branch information
ipetkov committed Aug 13, 2014
1 parent 51c7e20 commit 3fe0ba9
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/libnative/io/process.rs
Expand Up @@ -80,7 +80,7 @@ impl Process {
rtio::Ignored => { ret.push(None); Ok(None) }
rtio::InheritFd(fd) => {
ret.push(None);
Ok(Some(file::FileDesc::new(fd, true)))
Ok(Some(file::FileDesc::new(fd, false)))
}
rtio::CreatePipe(readable, _writable) => {
let (reader, writer) = try!(pipe());
Expand Down
25 changes: 24 additions & 1 deletion src/libstd/io/process.rs
Expand Up @@ -378,7 +378,8 @@ pub enum StdioContainer {
Ignored,

/// The specified file descriptor is inherited for the stream which it is
/// specified for.
/// specified for. Ownership of the file descriptor is *not* taken, so the
/// caller must clean it up.
InheritFd(libc::c_int),

/// Creates a pipe for the specified file descriptor which will be created
Expand Down Expand Up @@ -605,6 +606,7 @@ impl Drop for Process {

#[cfg(test)]
mod tests {
extern crate native;
use io::process::{Command, Process};
use prelude::*;

Expand Down Expand Up @@ -1017,4 +1019,25 @@ mod tests {
assert!(Process::kill(id, 0).is_ok());
assert!(Process::kill(id, PleaseExitSignal).is_ok());
})

iotest!(fn dont_close_fd_on_command_spawn() {
use std::rt::rtio::{Truncate, Write};
use native::io::file;

let path = if cfg!(windows) {
Path::new("NUL")
} else {
Path::new("/dev/null")
};

let mut fdes = match file::open(&path.to_c_str(), Truncate, Write) {
Ok(f) => f,
Err(_) => fail!("failed to open file descriptor"),
};

let mut cmd = pwd_cmd();
let _ = cmd.stdout(InheritFd(fdes.fd()));
assert!(cmd.status().unwrap().success());
assert!(fdes.inner_write("extra write\n".as_bytes()).is_ok());
})
}

5 comments on commit 3fe0ba9

@bors
Copy link
Contributor

@bors bors commented on 3fe0ba9 Aug 13, 2014

Choose a reason for hiding this comment

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

@bors
Copy link
Contributor

@bors bors commented on 3fe0ba9 Aug 13, 2014

Choose a reason for hiding this comment

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

merging ipetkov/rust/cmd-fd-fix-retry = 3fe0ba9 into auto

@bors
Copy link
Contributor

@bors bors commented on 3fe0ba9 Aug 13, 2014

Choose a reason for hiding this comment

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

ipetkov/rust/cmd-fd-fix-retry = 3fe0ba9 merged ok, testing candidate = 9d55421

@bors
Copy link
Contributor

@bors bors commented on 3fe0ba9 Aug 13, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 9d55421

Please sign in to comment.