From eb37bbcebc3f6d0981eef892817f3a4570e35907 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 19 May 2022 06:41:35 -0700 Subject: [PATCH 1/4] Document that `BorrowedFd` may be used to do a `dup`. --- library/std/src/os/unix/io/mod.rs | 28 +++++++++++++++++-------- library/std/src/os/windows/io/handle.rs | 6 +++--- library/std/src/os/windows/io/mod.rs | 6 ++++++ library/std/src/sys/windows/handle.rs | 2 +- 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/library/std/src/os/unix/io/mod.rs b/library/std/src/os/unix/io/mod.rs index e3a7cfc8d2ab8..7556d3ad0b230 100644 --- a/library/std/src/os/unix/io/mod.rs +++ b/library/std/src/os/unix/io/mod.rs @@ -26,20 +26,30 @@ //! that they don't outlive the resource they point to. These are safe to //! use. `BorrowedFd` values may be used in APIs which provide safe access to //! any system call except for: +//! //! - `close`, because that would end the dynamic lifetime of the resource //! without ending the lifetime of the file descriptor. +//! //! - `dup2`/`dup3`, in the second argument, because this argument is //! closed and assigned a new resource, which may break the assumptions //! other code using that file descriptor. -//! This list doesn't include `mmap`, since `mmap` does do a proper borrow of -//! its file descriptor argument. That said, `mmap` is unsafe for other -//! reasons: it operates on raw pointers, and it can have undefined behavior if -//! the underlying storage is mutated. Mutations may come from other processes, -//! or from the same process if the API provides `BorrowedFd` access, since as -//! mentioned earlier, `BorrowedFd` values may be used in APIs which provide -//! safe access to any system call. Consequently, code using `mmap` and -//! presenting a safe API must take full responsibility for ensuring that safe -//! Rust code cannot evoke undefined behavior through it. +//! +//! `BorrowedFd` values may be used in APIs which provide safe access to `dup` +//! system calls, so types implementing `AsFd` or `From` should not +//! assume they always have exclusive access to the underlying file +//! description. +//! +//! `BorrowedFd` values may also be used with `mmap`, since `mmap` uses the +//! provided file descriptor in a manner similar to `dup` and does not require +//! the `BorrowedFd` passed to it to live for the lifetime of the resulting +//! mapping. That said, `mmap` is unsafe for other reasons: it operates on raw +//! pointers, and it can have undefined behavior if the underlying storage is +//! mutated. Mutations may come from other processes, or from the same process +//! if the API provides `BorrowedFd` access, since as mentioned earlier, +//! `BorrowedFd` values may be used in APIs which provide safe access to any +//! system call. Consequently, code using `mmap` and presenting a safe API must +//! take full responsibility for ensuring that safe Rust code cannot evoke +//! undefined behavior through it. //! //! Like boxes, `OwnedFd` values conceptually own the resource they point to, //! and free (close) it when they are dropped. diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 2f7c07c791051..91b886c0888ee 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -189,7 +189,7 @@ impl OwnedHandle { access: c::DWORD, inherit: bool, options: c::DWORD, - ) -> io::Result { + ) -> io::Result { let handle = self.as_raw_handle(); // `Stdin`, `Stdout`, and `Stderr` can all hold null handles, such as @@ -197,7 +197,7 @@ impl OwnedHandle { // if we passed it a null handle, but we can treat null as a valid // handle which doesn't do any I/O, and allow it to be duplicated. if handle.is_null() { - return unsafe { Ok(Self::from_raw_handle(handle)) }; + return unsafe { Ok(OwnedHandle::from_raw_handle(handle)) }; } let mut ret = ptr::null_mut(); @@ -213,7 +213,7 @@ impl OwnedHandle { options, ) })?; - unsafe { Ok(Self::from_raw_handle(ret)) } + unsafe { Ok(OwnedHandle::from_raw_handle(ret)) } } } diff --git a/library/std/src/os/windows/io/mod.rs b/library/std/src/os/windows/io/mod.rs index 315450597076f..e2a401fb6962b 100644 --- a/library/std/src/os/windows/io/mod.rs +++ b/library/std/src/os/windows/io/mod.rs @@ -36,6 +36,12 @@ //! dynamic lifetime of the resource without ending the lifetime of the //! handle or socket. //! +//! `BorrowedHandle` and `BorrowedSocket` values may be used in APIs which +//! provide safe access to `DuplicateHandle` and `WSADuplicateSocketW` and +//! related functions, so types implementing `AsHandle`, `AsSocket`, +//! `From`, or `From` should not assume they always +//! have exclusive access to the underlying object. +//! //! Like boxes, `OwnedHandle` and `OwnedSocket` values conceptually own the //! resource they point to, and free (close) it when they are dropped. //! diff --git a/library/std/src/sys/windows/handle.rs b/library/std/src/sys/windows/handle.rs index ef9a8bd690031..1e7b6e1eab03a 100644 --- a/library/std/src/sys/windows/handle.rs +++ b/library/std/src/sys/windows/handle.rs @@ -218,7 +218,7 @@ impl Handle { inherit: bool, options: c::DWORD, ) -> io::Result { - Ok(Self(self.0.duplicate(access, inherit, options)?)) + Ok(Self(self.0.as_handle().duplicate(access, inherit, options)?)) } /// Performs a synchronous read. From 5d0eae81aecb71d70d620525459972bf36771921 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 19 May 2022 06:43:37 -0700 Subject: [PATCH 2/4] Add `BorrowedFd::try_clone_to_owned`. And `BorrowedHandle::try_clone_to_owned` and `BorrowedSocket::try_clone_to_owned` on Windows. --- library/std/src/os/fd/owned.rs | 15 ++++++++--- library/std/src/os/windows/io/handle.rs | 8 ++++++ library/std/src/os/windows/io/socket.rs | 36 +++++++++++++++---------- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/library/std/src/os/fd/owned.rs b/library/std/src/os/fd/owned.rs index 45b792039efb7..7d688940d45ff 100644 --- a/library/std/src/os/fd/owned.rs +++ b/library/std/src/os/fd/owned.rs @@ -79,9 +79,18 @@ impl BorrowedFd<'_> { impl OwnedFd { /// Creates a new `OwnedFd` instance that shares the same underlying file handle /// as the existing `OwnedFd` instance. - #[cfg(not(target_arch = "wasm32"))] #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone(&self) -> crate::io::Result { + self.as_fd().try_clone_to_owned() + } +} + +impl BorrowedFd<'_> { + /// Creates a new `OwnedFd` instance that shares the same underlying file + /// description as the existing `BorrowedFd` instance. + #[cfg(not(target_arch = "wasm32"))] + #[stable(feature = "io_safety", since = "1.63.0")] + pub fn try_clone_to_owned(&self) -> crate::io::Result { // We want to atomically duplicate this file descriptor and set the // CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This // is a POSIX flag that was added to Linux in 2.6.24. @@ -96,12 +105,12 @@ impl OwnedFd { let cmd = libc::F_DUPFD; let fd = cvt(unsafe { libc::fcntl(self.as_raw_fd(), cmd, 0) })?; - Ok(unsafe { Self::from_raw_fd(fd) }) + Ok(unsafe { OwnedFd::from_raw_fd(fd) }) } #[cfg(target_arch = "wasm32")] #[stable(feature = "io_safety", since = "1.63.0")] - pub fn try_clone(&self) -> crate::io::Result { + pub fn try_clone_to_owned(&self) -> crate::io::Result { Err(crate::io::const_io_error!( crate::io::ErrorKind::Unsupported, "operation not supported on WASI yet", diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 91b886c0888ee..c9d8624e838cc 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -181,6 +181,14 @@ impl OwnedHandle { /// as the existing `OwnedHandle` instance. #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone(&self) -> crate::io::Result { + self.as_handle().try_clone_to_owned() + } +} + +impl BorrowedHandle<'_> { + /// Creates a new `OwnedHandle` instance that shares the same underlying + /// object as the existing `BorrowedHandle` instance. + pub fn try_clone_to_owned(&self) -> crate::io::Result { self.duplicate(0, false, c::DUPLICATE_SAME_ACCESS) } diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index c5a381c40d243..b8cacf0b6c560 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -86,6 +86,28 @@ impl OwnedSocket { /// as the existing `OwnedSocket` instance. #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone(&self) -> io::Result { + self.as_socket().try_clone_to_owned() + } + + // FIXME(strict_provenance_magic): we defined RawSocket to be a u64 ;-; + #[cfg(not(target_vendor = "uwp"))] + pub(crate) fn set_no_inherit(&self) -> io::Result<()> { + cvt(unsafe { + c::SetHandleInformation(self.as_raw_socket() as c::HANDLE, c::HANDLE_FLAG_INHERIT, 0) + }) + .map(drop) + } + + #[cfg(target_vendor = "uwp")] + pub(crate) fn set_no_inherit(&self) -> io::Result<()> { + Err(io::const_io_error!(io::ErrorKind::Unsupported, "Unavailable on UWP")) + } +} + +impl BorrowedSocket<'_> { + /// Creates a new `OwnedSocket` instance that shares the same underlying + /// object as the existing `BorrowedSocket` instance. + pub fn try_clone_to_owned(&self) -> io::Result { let mut info = unsafe { mem::zeroed::() }; let result = unsafe { c::WSADuplicateSocketW(self.as_raw_socket(), c::GetCurrentProcessId(), &mut info) @@ -133,20 +155,6 @@ impl OwnedSocket { } } } - - // FIXME(strict_provenance_magic): we defined RawSocket to be a u64 ;-; - #[cfg(not(target_vendor = "uwp"))] - pub(crate) fn set_no_inherit(&self) -> io::Result<()> { - cvt(unsafe { - c::SetHandleInformation(self.as_raw_socket() as c::HANDLE, c::HANDLE_FLAG_INHERIT, 0) - }) - .map(drop) - } - - #[cfg(target_vendor = "uwp")] - pub(crate) fn set_no_inherit(&self) -> io::Result<()> { - Err(io::const_io_error!(io::ErrorKind::Unsupported, "Unavailable on UWP")) - } } /// Returns the last error from the Windows socket interface. From 007cbfd1db63400ad9bebb443080a999562af15e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 19 May 2022 06:59:12 -0700 Subject: [PATCH 3/4] Revise the documentation for `try_clone`. On Unix, describe these in terms of the underlying "file description". On Windows, describe them in terms of the underlying "object". --- library/std/src/os/fd/owned.rs | 6 ++++-- library/std/src/os/windows/io/handle.rs | 4 ++-- library/std/src/os/windows/io/socket.rs | 4 ++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/library/std/src/os/fd/owned.rs b/library/std/src/os/fd/owned.rs index 7d688940d45ff..dd965ddc01eef 100644 --- a/library/std/src/os/fd/owned.rs +++ b/library/std/src/os/fd/owned.rs @@ -77,8 +77,8 @@ impl BorrowedFd<'_> { } impl OwnedFd { - /// Creates a new `OwnedFd` instance that shares the same underlying file handle - /// as the existing `OwnedFd` instance. + /// Creates a new `OwnedFd` instance that shares the same underlying file + /// description as the existing `OwnedFd` instance. #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone(&self) -> crate::io::Result { self.as_fd().try_clone_to_owned() @@ -108,6 +108,8 @@ impl BorrowedFd<'_> { Ok(unsafe { OwnedFd::from_raw_fd(fd) }) } + /// Creates a new `OwnedFd` instance that shares the same underlying file + /// description as the existing `BorrowedFd` instance. #[cfg(target_arch = "wasm32")] #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone_to_owned(&self) -> crate::io::Result { diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index c9d8624e838cc..9ca5f3cc16838 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -177,8 +177,8 @@ impl TryFrom for OwnedHandle { } impl OwnedHandle { - /// Creates a new `OwnedHandle` instance that shares the same underlying file handle - /// as the existing `OwnedHandle` instance. + /// Creates a new `OwnedHandle` instance that shares the same underlying + /// object as the existing `OwnedHandle` instance. #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone(&self) -> crate::io::Result { self.as_handle().try_clone_to_owned() diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index b8cacf0b6c560..3eafb13826573 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -82,8 +82,8 @@ impl BorrowedSocket<'_> { } impl OwnedSocket { - /// Creates a new `OwnedSocket` instance that shares the same underlying socket - /// as the existing `OwnedSocket` instance. + /// Creates a new `OwnedSocket` instance that shares the same underlying + /// object as the existing `OwnedSocket` instance. #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone(&self) -> io::Result { self.as_socket().try_clone_to_owned() From ee49d65fc35d968e328ab63cc8330c1a43088bd2 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Wed, 15 Jun 2022 09:46:56 -0700 Subject: [PATCH 4/4] Add the new stability attributes, for Windows. --- library/std/src/os/windows/io/handle.rs | 1 + library/std/src/os/windows/io/socket.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs index 9ca5f3cc16838..16cc8fa2783ee 100644 --- a/library/std/src/os/windows/io/handle.rs +++ b/library/std/src/os/windows/io/handle.rs @@ -188,6 +188,7 @@ impl OwnedHandle { impl BorrowedHandle<'_> { /// Creates a new `OwnedHandle` instance that shares the same underlying /// object as the existing `BorrowedHandle` instance. + #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone_to_owned(&self) -> crate::io::Result { self.duplicate(0, false, c::DUPLICATE_SAME_ACCESS) } diff --git a/library/std/src/os/windows/io/socket.rs b/library/std/src/os/windows/io/socket.rs index 3eafb13826573..72cb3406dcada 100644 --- a/library/std/src/os/windows/io/socket.rs +++ b/library/std/src/os/windows/io/socket.rs @@ -107,6 +107,7 @@ impl OwnedSocket { impl BorrowedSocket<'_> { /// Creates a new `OwnedSocket` instance that shares the same underlying /// object as the existing `BorrowedSocket` instance. + #[stable(feature = "io_safety", since = "1.63.0")] pub fn try_clone_to_owned(&self) -> io::Result { let mut info = unsafe { mem::zeroed::() }; let result = unsafe {