From 4f8a50814cda201902f7fe2a903f6b7ea4f71d87 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Fri, 18 Oct 2024 08:20:38 -0700 Subject: [PATCH] Add safety comments for `mmap` etc. (#1188) * Add safety comments for `mmap` etc. Add proper safety comments for `mmap` and related functions. Many of the tricky things around `mmap` are about handing out Rust references to mapped memory, and that technically isn't rustix's responsibility to worry about, so these safety comments can be pretty simple. And, several other miscellaneous documentation and comment cleanups. * Document the new safety precondition for `split_init`. --- .github/workflows/test-users.yml | 22 +++---- Cargo.toml | 2 +- src/backend/libc/mod.rs | 2 +- src/backend/libc/net/addr.rs | 2 +- src/backend/linux_raw/vdso_wrappers.rs | 12 ++-- src/buffer.rs | 3 +- src/event/poll.rs | 2 +- src/mm/mmap.rs | 86 ++++++++++++++++++-------- src/net/send_recv/msg.rs | 5 ++ src/process/procctl.rs | 9 ++- 10 files changed, 93 insertions(+), 52 deletions(-) diff --git a/.github/workflows/test-users.yml b/.github/workflows/test-users.yml index d951fda4c..f86eaa71d 100644 --- a/.github/workflows/test-users.yml +++ b/.github/workflows/test-users.yml @@ -112,7 +112,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -281,7 +281,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -437,7 +437,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -622,7 +622,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -773,7 +773,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -924,7 +924,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -1075,7 +1075,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -1226,7 +1226,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -1382,7 +1382,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -1538,7 +1538,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched @@ -1759,7 +1759,7 @@ jobs: rustup target add ${{ matrix.target }} if: matrix.target != '' - - uses: actions/cache@v3 + - uses: actions/cache@v4 with: path: ${{ runner.tool_cache }}/qemu key: qemu-${{ matrix.target }}-${{ env.QEMU_BUILD_VERSION }}-patched diff --git a/Cargo.toml b/Cargo.toml index 829eccbd1..42fe2b4d9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ itoa = { version = "1.0.1", default-features = false, optional = true } # Special dependencies used in rustc-dep-of-std mode. core = { version = "1.0.0", optional = true, package = "rustc-std-workspace-core" } -rustc-std-workspace-alloc = { version = "1.0.0", optional = true } # not aliased here but in lib.rs casuse of name collision with the alloc feature +rustc-std-workspace-alloc = { version = "1.0.0", optional = true } # not aliased here but in lib.rs because of name collision with the alloc feature compiler_builtins = { version = '0.1.49', optional = true } # The procfs feature needs once_cell. diff --git a/src/backend/libc/mod.rs b/src/backend/libc/mod.rs index 9ecb09f12..56b4fe168 100644 --- a/src/backend/libc/mod.rs +++ b/src/backend/libc/mod.rs @@ -78,7 +78,7 @@ pub(crate) mod fd { /// /// [`AsFd`]: https://doc.rust-lang.org/stable/std/os/fd/trait.AsFd.html pub trait AsFd { - /// An `as_fd` function for Winsock, where a `Fd` is a `Socket`. + /// An `as_fd` function for Winsock, where an `Fd` is a `Socket`. fn as_fd(&self) -> BorrowedFd; } impl AsFd for T { diff --git a/src/backend/libc/net/addr.rs b/src/backend/libc/net/addr.rs index bef45e44e..7a26eea7b 100644 --- a/src/backend/libc/net/addr.rs +++ b/src/backend/libc/net/addr.rs @@ -74,7 +74,7 @@ impl SocketAddrUnix { }) } - fn init() -> c::sockaddr_un { + const fn init() -> c::sockaddr_un { c::sockaddr_un { #[cfg(any( bsd, diff --git a/src/backend/linux_raw/vdso_wrappers.rs b/src/backend/linux_raw/vdso_wrappers.rs index bd539e96c..00ed694f6 100644 --- a/src/backend/linux_raw/vdso_wrappers.rs +++ b/src/backend/linux_raw/vdso_wrappers.rs @@ -530,9 +530,9 @@ fn init() { if ok { assert!(!ptr.is_null()); - // Store the computed function addresses in static - // storage so that we don't need to compute it again (but if - // we do, it doesn't hurt anything). + // Store the computed function addresses in static storage so + // that we don't need to compute them again (but if we do, it + // doesn't hurt anything). CLOCK_GETTIME.store(ptr.cast(), Relaxed); } } @@ -583,9 +583,9 @@ fn init() { if ok { assert!(!ptr.is_null()); - // Store the computed function addresses in static - // storage so that we don't need to compute it again (but if - // we do, it doesn't hurt anything). + // Store the computed function addresses in static storage so + // that we don't need to compute them again (but if we do, it + // doesn't hurt anything). GETCPU.store(ptr.cast(), Relaxed); } } diff --git a/src/buffer.rs b/src/buffer.rs index 648b4c7ad..bb31ac056 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -9,7 +9,8 @@ use core::slice; /// /// # Safety /// -/// At least `init_len` bytes must be initialized. +/// `init_len` must not be greater than `buf.len()`, and at least `init_len` +/// bytes must be initialized. #[inline] pub(super) unsafe fn split_init( buf: &mut [MaybeUninit], diff --git a/src/event/poll.rs b/src/event/poll.rs index 8c86a0f3f..c1bbd54ca 100644 --- a/src/event/poll.rs +++ b/src/event/poll.rs @@ -7,7 +7,7 @@ pub use backend::event::poll_fd::{PollFd, PollFlags}; /// On macOS, `poll` doesn't work on fds for /dev/tty or /dev/null, however /// [`select`] is available and does work on these fds. /// -/// [`select`]: crate::event::select +/// [`select`]: crate::event::select() /// /// # References /// - [Beej's Guide to Network Programming] diff --git a/src/mm/mmap.rs b/src/mm/mmap.rs index afe6b152e..475e00332 100644 --- a/src/mm/mmap.rs +++ b/src/mm/mmap.rs @@ -3,7 +3,7 @@ //! # Safety //! //! `mmap` and related functions manipulate raw pointers and have special -//! semantics and are wildly unsafe. +//! semantics. #![allow(unsafe_code)] use crate::{backend, io}; @@ -52,7 +52,16 @@ impl MapFlags { /// /// # Safety /// -/// Raw pointers and lots of special semantics. +/// If `ptr` is not null, it must be aligned to the applicable page size, and +/// the range of memory starting at `ptr` and extending for `len` bytes, +/// rounded up to the applicable page size, must be valid to mutate using +/// `ptr`'s provenance. +/// +/// If there exist any Rust references referring to the memory region, or if +/// you subsequently create a Rust reference referring to the resulting region, +/// it is your responsibility to ensure that the Rust reference invariants are +/// preserved, including ensuring that the memory is not mutated in a way that +/// a Rust reference would not expect. /// /// # References /// - [POSIX] @@ -93,7 +102,10 @@ pub unsafe fn mmap( /// /// # Safety /// -/// Raw pointers and lots of special semantics. +/// If `ptr` is not null, it must be aligned to the applicable page size, and +/// the range of memory starting at `ptr` and extending for `len` bytes, +/// rounded up to the applicable page size, must be valid to mutate with +/// `ptr`'s provenance. /// /// # References /// - [POSIX] @@ -130,7 +142,10 @@ pub unsafe fn mmap_anonymous( /// /// # Safety /// -/// Raw pointers and lots of special semantics. +/// `ptr` must be aligned to the applicable page size, and the range of memory +/// starting at `ptr` and extending for `len` bytes, rounded up to the +/// applicable page size, must be valid to mutate with `ptr`'s provenance. And +/// there must be no Rust references referring to that memory. /// /// # References /// - [POSIX] @@ -165,7 +180,15 @@ pub unsafe fn munmap(ptr: *mut c_void, len: usize) -> io::Result<()> { /// /// # Safety /// -/// Raw pointers and lots of special semantics. +/// `old_address` must be aligned to the applicable page size, and the range of +/// memory starting at `old_address` and extending for `old_size` bytes, +/// rounded up to the applicable page size, must be valid to mutate with +/// `old_address`'s provenance. If `MremapFlags::MAY_MOVE` is set in `flags`, +/// there must be no Rust references referring to that the memory. +/// +/// If `new_size` is less than `old_size`, than there must be no Rust +/// references referring to the memory starting at offset `new_size` and ending +/// at `old_size`. /// /// # References /// - [Linux] @@ -190,7 +213,16 @@ pub unsafe fn mremap( /// /// # Safety /// -/// Raw pointers and lots of special semantics. +/// `old_address` and `new_address` must be aligned to the applicable page +/// size, the range of memory starting at `old_address` and extending for +/// `old_size` bytes, rounded up to the applicable page size, must be valid to +/// mutate with `old_address`'s provenance, and the range of memory starting at +/// `new_address` and extending for `new_size` bytes, rounded up to the +/// applicable page size, must be valid to mutate with `new_address`'s +/// provenance. +/// +/// There must be no Rust references referring to either of those memory +/// regions. /// /// # References /// - [Linux] @@ -214,7 +246,9 @@ pub unsafe fn mremap_fixed( /// /// # Safety /// -/// Raw pointers and lots of special semantics. +/// The range of memory starting at `ptr` and extending for `len` bytes, +/// rounded up to the applicable page size, must be valid to read with `ptr`'s +/// provenance. /// /// # References /// - [POSIX] @@ -241,18 +275,17 @@ pub unsafe fn mprotect(ptr: *mut c_void, len: usize, flags: MprotectFlags) -> io /// `mlock(ptr, len)`—Lock memory into RAM. /// -/// # Safety -/// -/// This function operates on raw pointers, but it should only be used on -/// memory which the caller owns. Technically, locking memory shouldn't violate -/// any invariants, but since unlocking it can violate invariants, this -/// function is also unsafe for symmetry. -/// /// Some implementations implicitly round the memory region out to the nearest /// page boundaries, so this function may lock more memory than explicitly /// requested if the memory isn't page-aligned. Other implementations fail if /// the memory isn't page-aligned. /// +/// # Safety +/// +/// The range of memory starting at `ptr`, rounded down to the applicable page +/// boundary, and extending for `len` bytes, rounded up to the applicable page +/// size, must be valid to read with `ptr`'s provenance. +/// /// # References /// - [POSIX] /// - [Linux] @@ -282,17 +315,16 @@ pub unsafe fn mlock(ptr: *mut c_void, len: usize) -> io::Result<()> { /// /// `mlock_with` is the same as [`mlock`] but adds an additional flags operand. /// -/// # Safety -/// -/// This function operates on raw pointers, but it should only be used on -/// memory which the caller owns. Technically, locking memory shouldn't violate -/// any invariants, but since unlocking it can violate invariants, this -/// function is also unsafe for symmetry. -/// /// Some implementations implicitly round the memory region out to the nearest /// page boundaries, so this function may lock more memory than explicitly /// requested if the memory isn't page-aligned. /// +/// # Safety +/// +/// The range of memory starting at `ptr`, rounded down to the applicable page +/// boundary, and extending for `len` bytes, rounded up to the applicable page +/// size, must be valid to read with `ptr`'s provenance. +/// /// # References /// - [Linux] /// - [glibc] @@ -308,16 +340,16 @@ pub unsafe fn mlock_with(ptr: *mut c_void, len: usize, flags: MlockFlags) -> io: /// `munlock(ptr, len)`—Unlock memory. /// -/// # Safety -/// -/// This function operates on raw pointers, but it should only be used on -/// memory which the caller owns, to avoid compromising the `mlock` invariants -/// of other unrelated code in the process. -/// /// Some implementations implicitly round the memory region out to the nearest /// page boundaries, so this function may unlock more memory than explicitly /// requested if the memory isn't page-aligned. /// +/// # Safety +/// +/// The range of memory starting at `ptr`, rounded down to the applicable page +/// boundary, and extending for `len` bytes, rounded up to the applicable page +/// size, must be valid to read with `ptr`'s provenance. +/// /// # References /// - [POSIX] /// - [Linux] diff --git a/src/net/send_recv/msg.rs b/src/net/send_recv/msg.rs index 794485d9f..6e145136f 100644 --- a/src/net/send_recv/msg.rs +++ b/src/net/send_recv/msg.rs @@ -593,6 +593,11 @@ impl FusedIterator for AncillaryDrain<'_> {} /// `sendmsg(msghdr)`—Sends a message on a socket. /// +/// This function is for use on connected sockets, as it doesn't have +/// a way to specify an address. See the [`sendmsg_v4`], [`sendmsg_v6`] +/// [`sendmsg_unix`], [`sendmsg_xdp`], and [`sendmsg_any`] to send +/// messages on unconnected sockets. +/// /// # References /// - [POSIX] /// - [Linux] diff --git a/src/process/procctl.rs b/src/process/procctl.rs index e198d03b0..a56d116c8 100644 --- a/src/process/procctl.rs +++ b/src/process/procctl.rs @@ -134,9 +134,11 @@ pub enum DumpableBehavior { } /// Set the state of the `dumpable` attribute for the process indicated by -/// `idtype` and `id`. This determines whether the process can be traced and -/// whether core dumps are produced for the process upon delivery of a signal -/// whose default behavior is to produce a core dump. +/// `idtype` and `id`. +/// +/// This determines whether the process can be traced and whether core dumps +/// are produced for the process upon delivery of a signal whose default +/// behavior is to produce a core dump. /// /// This is similar to `set_dumpable_behavior` on Linux, with the exception /// that on FreeBSD there is an extra argument `process`. When `process` is set @@ -453,6 +455,7 @@ pub enum TrapCapBehavior { } /// Set the current value of the capability mode violation trapping behavior. +/// /// If this behavior is enabled, the kernel would deliver a [`Signal::Trap`] /// signal on any return from a system call that would result in a /// [`io::Errno::NOTCAPABLE`] or [`io::Errno::CAPMODE`] error.