From 475da53d562293553934d0e579e91f2bf3be30f7 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Sat, 22 Jan 2022 14:37:48 -0700 Subject: [PATCH 1/2] Better type safety for mqueue On some platforms, mqd_t is a pointer. That means code like the below can trigger a segfault. Fix it by defining a Newtype around mqd_t that prevents use-after-free and dangling pointer scenarios. ```rust fn invalid_mqd_t() { let mqd: libc::mqd_t = std::ptr::null_mut(); mq_close(mqd).unwrap(); } ``` Also, get test coverage for mqueue in CI on FreeBSD. --- .cirrus.yml | 1 + CHANGELOG.md | 5 ++++ src/mqueue.rs | 70 +++++++++++++++++++++++++++++++++++++------------ test/test_mq.rs | 22 ++++++++-------- 4 files changed, 70 insertions(+), 28 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index c6f4e0b2d2..38416655f2 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -40,6 +40,7 @@ task: freebsd_instance: image: freebsd-12-2-release-amd64 setup_script: + - kldload mqueuefs - fetch https://sh.rustup.rs -o rustup.sh - sh rustup.sh -y --profile=minimal --default-toolchain $TOOLCHAIN - . $HOME/.cargo/env diff --git a/CHANGELOG.md b/CHANGELOG.md index 418ea62ef8..1ae85e31a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,11 @@ This project adheres to [Semantic Versioning](https://semver.org/). (#[1636](https://github.com/nix-rust/nix/pull/1636)) ### Changed + +- `mqueue` functions now operate on a distinct type, `nix::mqueue::MqdT`. + Accessors take this type by reference, not by value. + (#[1639](https://github.com/nix-rust/nix/pull/1639)) + ### Fixed ### Removed diff --git a/src/mqueue.rs b/src/mqueue.rs index 20740b54f9..792a5d2293 100644 --- a/src/mqueue.rs +++ b/src/mqueue.rs @@ -1,12 +1,40 @@ //! Posix Message Queue functions //! +//! # Example +//! +// no_run because a kernel module may be required. +//! ```no_run +//! # use std::ffi::CString; +//! # use nix::mqueue::*; +//! use nix::sys::stat::Mode; +//! +//! const MSG_SIZE: mq_attr_member_t = 32; +//! let mq_name= CString::new("/a_nix_test_queue").unwrap(); +//! +//! let oflag0 = MQ_OFlag::O_CREAT | MQ_OFlag::O_WRONLY; +//! let mode = Mode::S_IWUSR | Mode::S_IRUSR | Mode::S_IRGRP | Mode::S_IROTH; +//! let mqd0 = mq_open(&mq_name, oflag0, mode, None).unwrap(); +//! let msg_to_send = b"msg_1"; +//! mq_send(&mqd0, msg_to_send, 1).unwrap(); +//! +//! let oflag1 = MQ_OFlag::O_CREAT | MQ_OFlag::O_RDONLY; +//! let mqd1 = mq_open(&mq_name, oflag1, mode, None).unwrap(); +//! let mut buf = [0u8; 32]; +//! let mut prio = 0u32; +//! let len = mq_receive(&mqd1, &mut buf, &mut prio).unwrap(); +//! assert_eq!(prio, 1); +//! assert_eq!(msg_to_send, &buf[0..len]); +//! +//! mq_close(mqd1).unwrap(); +//! mq_close(mqd0).unwrap(); +//! ``` //! [Further reading and details on the C API](https://man7.org/linux/man-pages/man7/mq_overview.7.html) use crate::Result; use crate::errno::Errno; use libc::{self, c_char, mqd_t, size_t}; -use std::ffi::CString; +use std::ffi::CStr; use crate::sys::stat::Mode; use std::mem; @@ -34,6 +62,14 @@ pub struct MqAttr { mq_attr: libc::mq_attr, } +/// Identifies an open POSIX Message Queue +// A safer wrapper around libc::mqd_t, which is a pointer on some platforms +// Deliberately is not Clone to prevent use-after-close scenarios +#[repr(transparent)] +#[derive(Debug)] +#[allow(missing_copy_implementations)] +pub struct MqdT(mqd_t); + // x32 compatibility // See https://sourceware.org/bugzilla/show_bug.cgi?id=21279 #[cfg(all(target_arch = "x86_64", target_pointer_width = "32"))] @@ -87,11 +123,11 @@ impl MqAttr { /// See also [`mq_open(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_open.html) // The mode.bits cast is only lossless on some OSes #[allow(clippy::cast_lossless)] -pub fn mq_open(name: &CString, +pub fn mq_open(name: &CStr, oflag: MQ_OFlag, mode: Mode, attr: Option<&MqAttr>) - -> Result { + -> Result { let res = match attr { Some(mq_attr) => unsafe { libc::mq_open(name.as_ptr(), @@ -101,13 +137,13 @@ pub fn mq_open(name: &CString, }, None => unsafe { libc::mq_open(name.as_ptr(), oflag.bits()) }, }; - Errno::result(res) + Errno::result(res).map(MqdT) } /// Remove a message queue /// /// See also [`mq_unlink(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_unlink.html) -pub fn mq_unlink(name: &CString) -> Result<()> { +pub fn mq_unlink(name: &CStr) -> Result<()> { let res = unsafe { libc::mq_unlink(name.as_ptr()) }; Errno::result(res).map(drop) } @@ -115,18 +151,18 @@ pub fn mq_unlink(name: &CString) -> Result<()> { /// Close a message queue /// /// See also [`mq_close(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_close.html) -pub fn mq_close(mqdes: mqd_t) -> Result<()> { - let res = unsafe { libc::mq_close(mqdes) }; +pub fn mq_close(mqdes: MqdT) -> Result<()> { + let res = unsafe { libc::mq_close(mqdes.0) }; Errno::result(res).map(drop) } /// Receive a message from a message queue /// /// See also [`mq_receive(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_receive.html) -pub fn mq_receive(mqdes: mqd_t, message: &mut [u8], msg_prio: &mut u32) -> Result { +pub fn mq_receive(mqdes: &MqdT, message: &mut [u8], msg_prio: &mut u32) -> Result { let len = message.len() as size_t; let res = unsafe { - libc::mq_receive(mqdes, + libc::mq_receive(mqdes.0, message.as_mut_ptr() as *mut c_char, len, msg_prio as *mut u32) @@ -137,9 +173,9 @@ pub fn mq_receive(mqdes: mqd_t, message: &mut [u8], msg_prio: &mut u32) -> Resul /// Send a message to a message queue /// /// See also [`mq_send(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_send.html) -pub fn mq_send(mqdes: mqd_t, message: &[u8], msq_prio: u32) -> Result<()> { +pub fn mq_send(mqdes: &MqdT, message: &[u8], msq_prio: u32) -> Result<()> { let res = unsafe { - libc::mq_send(mqdes, + libc::mq_send(mqdes.0, message.as_ptr() as *const c_char, message.len(), msq_prio) @@ -150,9 +186,9 @@ pub fn mq_send(mqdes: mqd_t, message: &[u8], msq_prio: u32) -> Result<()> { /// Get message queue attributes /// /// See also [`mq_getattr(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_getattr.html) -pub fn mq_getattr(mqd: mqd_t) -> Result { +pub fn mq_getattr(mqd: &MqdT) -> Result { let mut attr = mem::MaybeUninit::::uninit(); - let res = unsafe { libc::mq_getattr(mqd, attr.as_mut_ptr()) }; + let res = unsafe { libc::mq_getattr(mqd.0, attr.as_mut_ptr()) }; Errno::result(res).map(|_| unsafe{MqAttr { mq_attr: attr.assume_init() }}) } @@ -161,10 +197,10 @@ pub fn mq_getattr(mqd: mqd_t) -> Result { /// It is recommend to use the `mq_set_nonblock()` and `mq_remove_nonblock()` convenience functions as they are easier to use /// /// [Further reading](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_setattr.html) -pub fn mq_setattr(mqd: mqd_t, newattr: &MqAttr) -> Result { +pub fn mq_setattr(mqd: &MqdT, newattr: &MqAttr) -> Result { let mut attr = mem::MaybeUninit::::uninit(); let res = unsafe { - libc::mq_setattr(mqd, &newattr.mq_attr as *const libc::mq_attr, attr.as_mut_ptr()) + libc::mq_setattr(mqd.0, &newattr.mq_attr as *const libc::mq_attr, attr.as_mut_ptr()) }; Errno::result(res).map(|_| unsafe{ MqAttr { mq_attr: attr.assume_init() }}) } @@ -173,7 +209,7 @@ pub fn mq_setattr(mqd: mqd_t, newattr: &MqAttr) -> Result { /// Sets the `O_NONBLOCK` attribute for a given message queue descriptor /// Returns the old attributes #[allow(clippy::useless_conversion)] // Not useless on all OSes -pub fn mq_set_nonblock(mqd: mqd_t) -> Result { +pub fn mq_set_nonblock(mqd: &MqdT) -> Result { let oldattr = mq_getattr(mqd)?; let newattr = MqAttr::new(mq_attr_member_t::from(MQ_OFlag::O_NONBLOCK.bits()), oldattr.mq_attr.mq_maxmsg, @@ -185,7 +221,7 @@ pub fn mq_set_nonblock(mqd: mqd_t) -> Result { /// Convenience function. /// Removes `O_NONBLOCK` attribute for a given message queue descriptor /// Returns the old attributes -pub fn mq_remove_nonblock(mqd: mqd_t) -> Result { +pub fn mq_remove_nonblock(mqd: &MqdT) -> Result { let oldattr = mq_getattr(mqd)?; let newattr = MqAttr::new(0, oldattr.mq_attr.mq_maxmsg, diff --git a/test/test_mq.rs b/test/test_mq.rs index 088ee7e0aa..8aff840ddc 100644 --- a/test/test_mq.rs +++ b/test/test_mq.rs @@ -41,13 +41,13 @@ fn test_mq_send_and_receive() { }; let mqd0 = r0.unwrap(); let msg_to_send = "msg_1"; - mq_send(mqd0, msg_to_send.as_bytes(), 1).unwrap(); + mq_send(&mqd0, msg_to_send.as_bytes(), 1).unwrap(); let oflag1 = MQ_OFlag::O_CREAT | MQ_OFlag::O_RDONLY; let mqd1 = mq_open(mq_name, oflag1, mode, Some(&attr)).unwrap(); let mut buf = [0u8; 32]; let mut prio = 0u32; - let len = mq_receive(mqd1, &mut buf, &mut prio).unwrap(); + let len = mq_receive(&mqd1, &mut buf, &mut prio).unwrap(); assert_eq!(prio, 1); mq_close(mqd1).unwrap(); @@ -71,7 +71,7 @@ fn test_mq_getattr() { }; let mqd = r.unwrap(); - let read_attr = mq_getattr(mqd).unwrap(); + let read_attr = mq_getattr(&mqd).unwrap(); assert_attr_eq!(read_attr, initial_attr); mq_close(mqd).unwrap(); } @@ -98,20 +98,20 @@ fn test_mq_setattr() { let mqd = r.unwrap(); let new_attr = MqAttr::new(0, 20, MSG_SIZE * 2, 100); - let old_attr = mq_setattr(mqd, &new_attr).unwrap(); + let old_attr = mq_setattr(&mqd, &new_attr).unwrap(); assert_attr_eq!(old_attr, initial_attr); // No changes here because according to the Linux man page only // O_NONBLOCK can be set (see tests below) #[cfg(not(any(target_os = "dragonfly", target_os = "netbsd")))] { - let new_attr_get = mq_getattr(mqd).unwrap(); + let new_attr_get = mq_getattr(&mqd).unwrap(); assert_ne!(new_attr_get, new_attr); } let new_attr_non_blocking = MqAttr::new(MQ_OFlag::O_NONBLOCK.bits() as mq_attr_member_t, 10, MSG_SIZE, 0); - mq_setattr(mqd, &new_attr_non_blocking).unwrap(); - let new_attr_get = mq_getattr(mqd).unwrap(); + mq_setattr(&mqd, &new_attr_non_blocking).unwrap(); + let new_attr_get = mq_getattr(&mqd).unwrap(); // now the O_NONBLOCK flag has been set #[cfg(not(any(target_os = "dragonfly", target_os = "netbsd")))] @@ -142,12 +142,12 @@ fn test_mq_set_nonblocking() { return; }; let mqd = r.unwrap(); - mq_set_nonblock(mqd).unwrap(); - let new_attr = mq_getattr(mqd); + mq_set_nonblock(&mqd).unwrap(); + let new_attr = mq_getattr(&mqd); let o_nonblock_bits = MQ_OFlag::O_NONBLOCK.bits() as mq_attr_member_t; assert_eq!(new_attr.unwrap().flags() & o_nonblock_bits, o_nonblock_bits); - mq_remove_nonblock(mqd).unwrap(); - let new_attr = mq_getattr(mqd); + mq_remove_nonblock(&mqd).unwrap(); + let new_attr = mq_getattr(&mqd); assert_eq!(new_attr.unwrap().flags() & o_nonblock_bits, 0); mq_close(mqd).unwrap(); } From 91049bc03be6a15f10f898fee89ad03a05bb886d Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Sat, 22 Jan 2022 14:46:07 -0700 Subject: [PATCH 2/2] Suppress clippy::not_unsafe_ptr_arg_deref warnings in ptrace on BSD Technically these functions don't violate Rust's safety rules, because libc::ptrace doesn't dereference those pointer args. Instead, it passes them directly to the kernel. --- src/sys/ptrace/bsd.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/sys/ptrace/bsd.rs b/src/sys/ptrace/bsd.rs index ac7d83126c..c4cc740396 100644 --- a/src/sys/ptrace/bsd.rs +++ b/src/sys/ptrace/bsd.rs @@ -167,6 +167,9 @@ pub fn step>>(pid: Pid, sig: T) -> Result<()> { } /// Reads a word from a processes memory at the given address +// Technically, ptrace doesn't dereference the pointer. It passes it directly +// to the kernel. +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn read(pid: Pid, addr: AddressType) -> Result { unsafe { // Traditionally there was a difference between reading data or @@ -176,6 +179,9 @@ pub fn read(pid: Pid, addr: AddressType) -> Result { } /// Writes a word into the processes memory at the given address +// Technically, ptrace doesn't dereference the pointer. It passes it directly +// to the kernel. +#[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn write(pid: Pid, addr: AddressType, data: c_int) -> Result<()> { unsafe { ptrace_other(Request::PT_WRITE_D, pid, addr, data).map(drop) } }