Skip to content

Add File::try_clone #31069

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

Merged
merged 1 commit into from
Feb 4, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
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
34 changes: 34 additions & 0 deletions src/libstd/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,18 @@ impl File {
pub fn metadata(&self) -> io::Result<Metadata> {
self.inner.file_attr().map(Metadata)
}

/// Creates a new independently owned handle to the underlying file.
///
/// The returned `File` is a reference to the same state that this object
/// references. Both handles will read and write with the same cursor
/// position.
#[unstable(feature = "file_try_clone", reason = "newly added", issue = "31405")]
pub fn try_clone(&self) -> io::Result<File> {
Ok(File {
inner: try!(self.inner.duplicate())
})
}
}

impl AsInner<fs_imp::File> for File {
Expand Down Expand Up @@ -2283,6 +2295,28 @@ mod tests {
assert!(v == &bytes[..]);
}

#[test]
fn file_try_clone() {
let tmpdir = tmpdir();

let mut f1 = check!(OpenOptions::new()
.read(true)
.write(true)
.create(true)
.open(&tmpdir.join("test")));
let mut f2 = check!(f1.try_clone());

check!(f1.write_all(b"hello world"));
check!(f1.seek(SeekFrom::Start(2)));

let mut buf = vec![];
check!(f2.read_to_end(&mut buf));
assert_eq!(buf, b"llo world");
drop(f2);

check!(f1.write_all(b"!"));
}

#[test]
#[cfg(not(windows))]
fn unlink_readonly() {
Expand Down
42 changes: 42 additions & 0 deletions src/libstd/sys/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use libc::{self, c_int, size_t, c_void};
use mem;
use sys::cvt;
use sys_common::AsInner;
use sync::atomic::{AtomicBool, Ordering};

pub struct FileDesc {
fd: c_int,
Expand Down Expand Up @@ -65,6 +66,47 @@ impl FileDesc {
debug_assert_eq!(ret, 0);
}
}

pub fn duplicate(&self) -> io::Result<FileDesc> {
// We want to atomically duplicate this file descriptor and set the
// CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This
// flag, however, isn't supported on older Linux kernels (earlier than
// 2.6.24).
//
// To detect this and ensure that CLOEXEC is still set, we
// follow a strategy similar to musl [1] where if passing
// F_DUPFD_CLOEXEC causes `fcntl` to return EINVAL it means it's not
// supported (the third parameter, 0, is always valid), so we stop
// trying that. We also *still* call the `set_cloexec` method as
// apparently some kernel at some point stopped setting CLOEXEC even
// though it reported doing so on F_DUPFD_CLOEXEC.
//
// Also note that Android doesn't have F_DUPFD_CLOEXEC, but get it to
// resolve so we at least compile this.
//
// [1]: http://comments.gmane.org/gmane.linux.lib.musl.general/2963
#[cfg(target_os = "android")]
use libc::F_DUPFD as F_DUPFD_CLOEXEC;
#[cfg(not(target_os = "android"))]
use libc::F_DUPFD_CLOEXEC;

let make_filedesc = |fd| {
let fd = FileDesc::new(fd);
fd.set_cloexec();
fd
};
static TRY_CLOEXEC: AtomicBool = AtomicBool::new(true);
let fd = self.raw();
if !cfg!(target_os = "android") && TRY_CLOEXEC.load(Ordering::Relaxed) {
match cvt(unsafe { libc::fcntl(fd, F_DUPFD_CLOEXEC, 0) }) {
Err(ref e) if e.raw_os_error() == Some(libc::EINVAL) => {
TRY_CLOEXEC.store(false, Ordering::Relaxed);
}
res => return res.map(make_filedesc),
}
}
cvt(unsafe { libc::fcntl(fd, libc::F_DUPFD, 0) }).map(make_filedesc)
}
}

impl AsInner<c_int> for FileDesc {
Expand Down
4 changes: 4 additions & 0 deletions src/libstd/sys/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,10 @@ impl File {
Ok(n as u64)
}

pub fn duplicate(&self) -> io::Result<File> {
self.0.duplicate().map(File)
}

pub fn fd(&self) -> &FileDesc { &self.0 }

pub fn into_fd(self) -> FileDesc { self.0 }
Expand Down
40 changes: 1 addition & 39 deletions src/libstd/sys/unix/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ use io;
use libc::{self, c_int, size_t};
use net::{SocketAddr, Shutdown};
use str;
use sync::atomic::{AtomicBool, Ordering};
use sys::fd::FileDesc;
use sys_common::{AsInner, FromInner, IntoInner};
use sys_common::net::{getsockopt, setsockopt};
Expand Down Expand Up @@ -67,44 +66,7 @@ impl Socket {
}

pub fn duplicate(&self) -> io::Result<Socket> {
// We want to atomically duplicate this file descriptor and set the
// CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This
// flag, however, isn't supported on older Linux kernels (earlier than
// 2.6.24).
//
// To detect this and ensure that CLOEXEC is still set, we
// follow a strategy similar to musl [1] where if passing
// F_DUPFD_CLOEXEC causes `fcntl` to return EINVAL it means it's not
// supported (the third parameter, 0, is always valid), so we stop
// trying that. We also *still* call the `set_cloexec` method as
// apparently some kernel at some point stopped setting CLOEXEC even
// though it reported doing so on F_DUPFD_CLOEXEC.
//
// Also note that Android doesn't have F_DUPFD_CLOEXEC, but get it to
// resolve so we at least compile this.
//
// [1]: http://comments.gmane.org/gmane.linux.lib.musl.general/2963
#[cfg(target_os = "android")]
use libc::F_DUPFD as F_DUPFD_CLOEXEC;
#[cfg(not(target_os = "android"))]
use libc::F_DUPFD_CLOEXEC;

let make_socket = |fd| {
let fd = FileDesc::new(fd);
fd.set_cloexec();
Socket(fd)
};
static TRY_CLOEXEC: AtomicBool = AtomicBool::new(true);
let fd = self.0.raw();
if !cfg!(target_os = "android") && TRY_CLOEXEC.load(Ordering::Relaxed) {
match cvt(unsafe { libc::fcntl(fd, F_DUPFD_CLOEXEC, 0) }) {
Err(ref e) if e.raw_os_error() == Some(libc::EINVAL) => {
TRY_CLOEXEC.store(false, Ordering::Relaxed);
}
res => return res.map(make_socket),
}
}
cvt(unsafe { libc::fcntl(fd, libc::F_DUPFD, 0) }).map(make_socket)
self.0.duplicate().map(Socket)
}

pub fn read(&self, buf: &mut [u8]) -> io::Result<usize> {
Expand Down
6 changes: 6 additions & 0 deletions src/libstd/sys/windows/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,12 @@ impl File {
Ok(newpos as u64)
}

pub fn duplicate(&self) -> io::Result<File> {
Ok(File {
handle: try!(self.handle.duplicate(0, true, c::DUPLICATE_SAME_ACCESS)),
Copy link
Contributor

@oconnor663 oconnor663 Oct 11, 2019

Choose a reason for hiding this comment

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

Asking 3.5 years after the fact: Does anyone know why we set inheritable = true here? I've run into a bug (because duct was abusing File::try_clone somewhat) where pipes get inherited by processes that aren't supposed to inherit them, when spawning happens on multiple threads at once. Normally the CREATE_PROCESS_LOCK prevents this, but this line here creates inheritable handles outside of that lock. duct is going to work around this by making all the appropriate system calls explicitly. But this inheritable flag might be cause some programs in the wild to keep files open longer than they should (even without a multi-threaded race condition like mine), and on Windows in particular that could cause bugs by making them un-deletable.

})
}

pub fn handle(&self) -> &Handle { &self.handle }

pub fn into_handle(self) -> Handle { self.handle }
Expand Down