Skip to content

Partial implementation of close for stdin/out/err (#40032) #46063

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/libstd/sys/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,30 @@ impl FileDesc {
}
cvt(unsafe { libc::fcntl(fd, libc::F_DUPFD, 0) }).and_then(make_filedesc)
}

/// Replace the open file referred to by `self` with the open file
/// referred to by `other`. This does not change the file
/// descriptor _number_ within self; rather, it changes what open
/// file that number refers to.
///
/// On success, the file formerly referred to by `self` is
/// transferred to a newly-allocated file descriptor and returned.
/// On failure, the file referred to by `self` is unchanged.
/// Regardless of success or failure, `other` is consumed, which
/// means the file descriptor formerly held by `other` will be
/// closed.
///
/// The file underlying `self` is replaced atomically, but the
/// _overall_ operation is not atomic; concurrent threads that snoop on
/// the set of valid file descriptors (which they shouldn't) can observe
/// intermediate states.
pub fn replace(&mut self, other: FileDesc) -> io::Result<FileDesc> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the replacement semantics are actually needed when closing, right? That is, I think it's safe for this to return () and avoid the first call to duplicate for the specific use case of Stdout::close?

Copy link
Contributor Author

@zackw zackw Nov 27, 2017

Choose a reason for hiding this comment

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

Yes, I realized the next morning that any caller that actually needs to preserve the old file can just call duplicate itself. And that will dovetail better with the semantics of SetStdHandle on Windows. I will make this change.

let previous = self.duplicate()?;
let fd = cvt(unsafe { libc::dup2(other.fd, self.fd) })?;

assert!(fd == self.fd);
Ok(previous)
}
}

impl<'a> Read for &'a FileDesc {
Expand Down
47 changes: 47 additions & 0 deletions src/libstd/sys/unix/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,25 @@
use io;
use libc;
use sys::fd::FileDesc;
use sys::fs::{File, OpenOptions};
use ffi::CStr;

pub struct Stdin(());
pub struct Stdout(());
pub struct Stderr(());

// FIXME: This duplicates code from process_common.rs.
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, would it be possible to extract the two to common code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, where should it live? File::open_bit_bucket maybe?

fn open_null_device (readable: bool) -> io::Result<FileDesc> {
let mut opts = OpenOptions::new();
opts.read(readable);
opts.write(!readable);
let path = unsafe {
CStr::from_ptr("/dev/null\0".as_ptr() as *const _)
};
let fd = File::open_c(&path, &opts)?;
Ok(fd.into_fd())
}

impl Stdin {
pub fn new() -> io::Result<Stdin> { Ok(Stdin(())) }

Expand All @@ -25,6 +39,21 @@ impl Stdin {
fd.into_raw();
ret
}

pub fn close() -> io::Result<()> {
// To close stdin, what we actually do is change its well-known
// file descriptor number to refer to a file open on the null
// device. This protects against code (perhaps in third-party
// libraries) that assumes STDIN_FILENO is always open and always
// refers to the same thing as stdin.
let mut fd = FileDesc::new(libc::STDIN_FILENO);
// If this step succeeds, the "previous" file descriptor returned
// by `fd.replace` is dropped and thus closed.
fd.replace(open_null_device(true)?)?;
// Don't close STDIN_FILENO itself, though!
fd.into_raw();
Copy link
Member

Choose a reason for hiding this comment

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

Errors in those prior ? will drop fd without calling your into_raw. Perhaps you could wrap it in ManuallyDrop instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good point. Thanks for the tip!

Ok(())
}
}

impl Stdout {
Expand All @@ -40,6 +69,15 @@ impl Stdout {
pub fn flush(&self) -> io::Result<()> {
Ok(())
}

pub fn close() -> io::Result<()> {
// See commentary for Stdin::close.

let mut fd = FileDesc::new(libc::STDOUT_FILENO);
fd.replace(open_null_device(false)?)?;
fd.into_raw();
Ok(())
}
}

impl Stderr {
Expand All @@ -55,6 +93,15 @@ impl Stderr {
pub fn flush(&self) -> io::Result<()> {
Ok(())
}

pub fn close() -> io::Result<()> {
// See commentary for Stdin::close.

let mut fd = FileDesc::new(libc::STDERR_FILENO);
fd.replace(open_null_device(false)?)?;
fd.into_raw();
Ok(())
}
}

// FIXME: right now this raw stderr handle is used in a few places because
Expand Down