Skip to content

Commit

Permalink
Add safety comments for mmap etc. (#1188)
Browse files Browse the repository at this point in the history
* 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`.
  • Loading branch information
sunfishcode authored Oct 18, 2024
1 parent 873bac5 commit ec96121
Show file tree
Hide file tree
Showing 10 changed files with 93 additions and 52 deletions.
22 changes: 11 additions & 11 deletions .github/workflows/test-users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/backend/libc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T: AsSocket> AsFd for T {
Expand Down
2 changes: 1 addition & 1 deletion src/backend/libc/net/addr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl SocketAddrUnix {
})
}

fn init() -> c::sockaddr_un {
const fn init() -> c::sockaddr_un {
c::sockaddr_un {
#[cfg(any(
bsd,
Expand Down
12 changes: 6 additions & 6 deletions src/backend/linux_raw/vdso_wrappers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u8>],
Expand Down
2 changes: 1 addition & 1 deletion src/event/poll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
86 changes: 59 additions & 27 deletions src/mm/mmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -93,7 +102,10 @@ pub unsafe fn mmap<Fd: AsFd>(
///
/// # 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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand Down
5 changes: 5 additions & 0 deletions src/net/send_recv/msg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
9 changes: 6 additions & 3 deletions src/process/procctl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit ec96121

Please sign in to comment.