From 38ded129236f112a7421f311aeb8ca750b661443 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Thu, 18 Apr 2024 20:03:45 +0200 Subject: [PATCH 1/3] Abort a process when FD ownership is violated When an EBADF happens then something else already touched an FD in ways it is not allowed to. At that point things can already be arbitrarily bad, e.g. clobbered mmaps. Recovery is not possible. All we can do is hasten the fire. --- library/std/src/os/fd/owned.rs | 15 ++++++++++++++- library/std/src/sys/pal/unix/fs.rs | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/library/std/src/os/fd/owned.rs b/library/std/src/os/fd/owned.rs index 010ce4e5076ba..8c421540af40e 100644 --- a/library/std/src/os/fd/owned.rs +++ b/library/std/src/os/fd/owned.rs @@ -176,7 +176,20 @@ impl Drop for OwnedFd { // something like EINTR), we might close another valid file descriptor // opened after we closed ours. #[cfg(not(target_os = "hermit"))] - let _ = libc::close(self.fd); + { + use crate::sys::os::errno; + // ideally this would use assert_unsafe_precondition!, but that's only in core + if cfg!(debug_assertions) { + // close() can bubble up error codes from FUSE which can send semantically + // inappropriate error codes including EBADF. + // So we check file flags instead which live on the file descriptor and not the underlying file. + // The downside is that it costs an extra syscall, so we only do it for debug. + if libc::fcntl(self.fd, libc::F_GETFD) == -1 && errno() == libc::EBADF { + rtabort!("IO Safety violation: owned file descriptor already closed"); + } + } + let _ = libc::close(self.fd); + } #[cfg(target_os = "hermit")] let _ = hermit_abi::close(self.fd); } diff --git a/library/std/src/sys/pal/unix/fs.rs b/library/std/src/sys/pal/unix/fs.rs index 3456155509ef3..2d57a9d81c911 100644 --- a/library/std/src/sys/pal/unix/fs.rs +++ b/library/std/src/sys/pal/unix/fs.rs @@ -870,6 +870,27 @@ impl Iterator for ReadDir { impl Drop for Dir { fn drop(&mut self) { + // ideally this would use assert_unsafe_precondition!, but that's only in core + #[cfg(all( + debug_assertions, + not(any( + target_os = "redox", + target_os = "nto", + target_os = "vita", + target_os = "hurd", + )) + ))] + { + use crate::sys::os::errno; + // close() can bubble up error codes from FUSE which can send semantically + // inappropriate error codes including EBADF. + // So we check file flags instead which live on the file descriptor and not the underlying file. + // The downside is that it costs an extra syscall, so we only do it for debug. + let fd = unsafe { libc::dirfd(self.0) }; + if unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1 && errno() == libc::EBADF { + rtabort!("IO Safety violation: DIR*'s owned file descriptor already closed"); + } + } let r = unsafe { libc::closedir(self.0) }; assert!( r == 0 || crate::io::Error::last_os_error().is_interrupted(), From 25babe9a79a2c5d35211e0d6bad348637ef2a246 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Sun, 21 Apr 2024 16:34:42 +0200 Subject: [PATCH 2/3] export assert_unsafe_precondition macro for std-internal use --- library/core/src/intrinsics.rs | 2 +- library/core/src/lib.rs | 4 +++- library/core/src/ub_checks.rs | 8 ++++++-- library/std/src/lib.rs | 1 + 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 9406efd7ab24a..7d2b84b986469 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -2720,7 +2720,7 @@ pub const unsafe fn typed_swap(x: *mut T, y: *mut T) { #[unstable(feature = "core_intrinsics", issue = "none")] #[inline(always)] #[cfg_attr(not(bootstrap), rustc_intrinsic)] // just make it a regular fn in bootstrap -pub(crate) const fn ub_checks() -> bool { +pub const fn ub_checks() -> bool { cfg!(debug_assertions) } diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs index 154565b6fee17..a7dcc4992f506 100644 --- a/library/core/src/lib.rs +++ b/library/core/src/lib.rs @@ -193,6 +193,7 @@ #![feature(str_split_inclusive_remainder)] #![feature(str_split_remainder)] #![feature(strict_provenance)] +#![feature(ub_checks)] #![feature(unchecked_shifts)] #![feature(utf16_extra)] #![feature(utf16_extra_const)] @@ -370,7 +371,8 @@ pub mod hint; pub mod intrinsics; pub mod mem; pub mod ptr; -mod ub_checks; +#[unstable(feature = "ub_checks", issue = "none")] +pub mod ub_checks; /* Core language traits */ diff --git a/library/core/src/ub_checks.rs b/library/core/src/ub_checks.rs index ff6b2d3053956..1aa6a288e7082 100644 --- a/library/core/src/ub_checks.rs +++ b/library/core/src/ub_checks.rs @@ -46,6 +46,8 @@ use crate::intrinsics::{self, const_eval_select}; /// variables cannot be optimized out in MIR, an innocent-looking `let` can produce enough /// debuginfo to have a measurable compile-time impact on debug builds. #[allow_internal_unstable(const_ub_checks)] // permit this to be called in stably-const fn +#[macro_export] +#[unstable(feature = "ub_checks", issue = "none")] macro_rules! assert_unsafe_precondition { ($kind:ident, $message:expr, ($($name:ident:$ty:ty = $arg:expr),*$(,)?) => $e:expr $(,)?) => { { @@ -75,11 +77,13 @@ macro_rules! assert_unsafe_precondition { } }; } -pub(crate) use assert_unsafe_precondition; +#[unstable(feature = "ub_checks", issue = "none")] +pub use assert_unsafe_precondition; /// Checking library UB is always enabled when UB-checking is done /// (and we use a reexport so that there is no unnecessary wrapper function). -pub(crate) use intrinsics::ub_checks as check_library_ub; +#[unstable(feature = "ub_checks", issue = "none")] +pub use intrinsics::ub_checks as check_library_ub; /// Determines whether we should check for language UB. /// diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index 241a443fab96e..3055c638e2cf7 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -358,6 +358,7 @@ #![feature(str_internals)] #![feature(strict_provenance)] #![feature(strict_provenance_atomic_ptr)] +#![feature(ub_checks)] // tidy-alphabetical-end // // Library features (alloc): From 1ba00d9cb2fcfef464b6a188fa3a7543c66eecaa Mon Sep 17 00:00:00 2001 From: The 8472 Date: Sun, 21 Apr 2024 17:19:15 +0200 Subject: [PATCH 3/3] put FD validity behind late debug_asserts checking uses the same machinery as assert_unsafe_precondition --- library/std/src/os/fd/owned.rs | 14 ++------- library/std/src/sys/pal/unix/fs.rs | 47 ++++++++++++++++++------------ 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/library/std/src/os/fd/owned.rs b/library/std/src/os/fd/owned.rs index 8c421540af40e..8c7fc4cb2e453 100644 --- a/library/std/src/os/fd/owned.rs +++ b/library/std/src/os/fd/owned.rs @@ -177,17 +177,9 @@ impl Drop for OwnedFd { // opened after we closed ours. #[cfg(not(target_os = "hermit"))] { - use crate::sys::os::errno; - // ideally this would use assert_unsafe_precondition!, but that's only in core - if cfg!(debug_assertions) { - // close() can bubble up error codes from FUSE which can send semantically - // inappropriate error codes including EBADF. - // So we check file flags instead which live on the file descriptor and not the underlying file. - // The downside is that it costs an extra syscall, so we only do it for debug. - if libc::fcntl(self.fd, libc::F_GETFD) == -1 && errno() == libc::EBADF { - rtabort!("IO Safety violation: owned file descriptor already closed"); - } - } + #[cfg(unix)] + crate::sys::fs::debug_assert_fd_is_open(self.fd); + let _ = libc::close(self.fd); } #[cfg(target_os = "hermit")] diff --git a/library/std/src/sys/pal/unix/fs.rs b/library/std/src/sys/pal/unix/fs.rs index 2d57a9d81c911..0a86d28c2e3b3 100644 --- a/library/std/src/sys/pal/unix/fs.rs +++ b/library/std/src/sys/pal/unix/fs.rs @@ -868,28 +868,39 @@ impl Iterator for ReadDir { } } +/// Aborts the process if a file desceriptor is not open, if debug asserts are enabled +/// +/// Many IO syscalls can't be fully trusted about EBADF error codes because those +/// might get bubbled up from a remote FUSE server rather than the file descriptor +/// in the current process being invalid. +/// +/// So we check file flags instead which live on the file descriptor and not the underlying file. +/// The downside is that it costs an extra syscall, so we only do it for debug. +#[inline] +pub(crate) fn debug_assert_fd_is_open(fd: RawFd) { + use crate::sys::os::errno; + + // this is similar to assert_unsafe_precondition!() but it doesn't require const + if core::ub_checks::check_library_ub() { + if unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1 && errno() == libc::EBADF { + rtabort!("IO Safety violation: owned file descriptor already closed"); + } + } +} + impl Drop for Dir { fn drop(&mut self) { - // ideally this would use assert_unsafe_precondition!, but that's only in core - #[cfg(all( - debug_assertions, - not(any( - target_os = "redox", - target_os = "nto", - target_os = "vita", - target_os = "hurd", - )) - ))] + // dirfd isn't supported everywhere + #[cfg(not(any( + miri, + target_os = "redox", + target_os = "nto", + target_os = "vita", + target_os = "hurd", + )))] { - use crate::sys::os::errno; - // close() can bubble up error codes from FUSE which can send semantically - // inappropriate error codes including EBADF. - // So we check file flags instead which live on the file descriptor and not the underlying file. - // The downside is that it costs an extra syscall, so we only do it for debug. let fd = unsafe { libc::dirfd(self.0) }; - if unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1 && errno() == libc::EBADF { - rtabort!("IO Safety violation: DIR*'s owned file descriptor already closed"); - } + debug_assert_fd_is_open(fd); } let r = unsafe { libc::closedir(self.0) }; assert!(