Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Suppress clippy::not_unsafe_ptr_arg_deref in mqueue, ptrace #1638

Closed
wants to merge 1 commit into from
Closed

Suppress clippy::not_unsafe_ptr_arg_deref in mqueue, ptrace #1638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/mqueue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ 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)
#[allow(clippy::not_unsafe_ptr_arg_deref)] // mqd_t is a pointer on some platforms
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, Clippy is right about this. Since mqd_t is a typedef, the user is allowed to do pretty much anything with it. As an example, this code will segfault:

#[test]
fn invalid_mqd_t() {
    let mqd: libc::mqd_t = std::ptr::null_mut();
    mq_close(mqd).unwrap();
}

I think Nix should fix this by defining a Newtype around mqd_t.

pub fn mq_close(mqdes: mqd_t) -> Result<()> {
let res = unsafe { libc::mq_close(mqdes) };
Errno::result(res).map(drop)
Expand All @@ -123,6 +124,7 @@ pub fn mq_close(mqdes: mqd_t) -> Result<()> {
/// Receive a message from a message queue
///
/// See also [`mq_receive(2)`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_receive.html)
#[allow(clippy::not_unsafe_ptr_arg_deref)] // mqd_t is a pointer on some platforms
pub fn mq_receive(mqdes: mqd_t, message: &mut [u8], msg_prio: &mut u32) -> Result<usize> {
let len = message.len() as size_t;
let res = unsafe {
Expand All @@ -137,6 +139,7 @@ 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)
#[allow(clippy::not_unsafe_ptr_arg_deref)] // mqd_t is a pointer on some platforms
pub fn mq_send(mqdes: mqd_t, message: &[u8], msq_prio: u32) -> Result<()> {
let res = unsafe {
libc::mq_send(mqdes,
Expand All @@ -150,6 +153,7 @@ 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)
#[allow(clippy::not_unsafe_ptr_arg_deref)] // mqd_t is a pointer on some platforms
pub fn mq_getattr(mqd: mqd_t) -> Result<MqAttr> {
let mut attr = mem::MaybeUninit::<libc::mq_attr>::uninit();
let res = unsafe { libc::mq_getattr(mqd, attr.as_mut_ptr()) };
Expand All @@ -161,6 +165,7 @@ pub fn mq_getattr(mqd: mqd_t) -> Result<MqAttr> {
/// 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)
#[allow(clippy::not_unsafe_ptr_arg_deref)] // mqd_t is a pointer on some platforms
pub fn mq_setattr(mqd: mqd_t, newattr: &MqAttr) -> Result<MqAttr> {
let mut attr = mem::MaybeUninit::<libc::mq_attr>::uninit();
let res = unsafe {
Expand All @@ -172,6 +177,7 @@ pub fn mq_setattr(mqd: mqd_t, newattr: &MqAttr) -> Result<MqAttr> {
/// Convenience function.
/// Sets the `O_NONBLOCK` attribute for a given message queue descriptor
/// Returns the old attributes
#[allow(clippy::not_unsafe_ptr_arg_deref)] // mqd_t is a pointer on some platforms
#[allow(clippy::useless_conversion)] // Not useless on all OSes
pub fn mq_set_nonblock(mqd: mqd_t) -> Result<MqAttr> {
let oldattr = mq_getattr(mqd)?;
Expand Down
8 changes: 5 additions & 3 deletions src/sys/ptrace/bsd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use crate::Result;
pub type RequestType = c_int;

cfg_if! {
if #[cfg(any(target_os = "dragonfly",
target_os = "freebsd",
if #[cfg(any(target_os = "dragonfly",
target_os = "freebsd",
target_os = "macos",
target_os = "openbsd"))] {
#[doc(hidden)]
Expand Down Expand Up @@ -120,7 +120,7 @@ pub fn cont<T: Into<Option<Signal>>>(pid: Pid, sig: T) -> Result<()> {

/// Issues a kill request as with `ptrace(PT_KILL, ...)`
///
/// This request is equivalent to `ptrace(PT_CONTINUE, ..., SIGKILL);`
/// This request is equivalent to `ptrace(PT_CONTINUE, ..., SIGKILL);`
pub fn kill(pid: Pid) -> Result<()> {
unsafe {
ptrace_other(Request::PT_KILL, pid, 0 as AddressType, 0).map(drop)
Expand Down Expand Up @@ -167,6 +167,7 @@ pub fn step<T: Into<Option<Signal>>>(pid: Pid, sig: T) -> Result<()> {
}

/// Reads a word from a processes memory at the given address
#[allow(clippy::not_unsafe_ptr_arg_deref)]
pub fn read(pid: Pid, addr: AddressType) -> Result<c_int> {
unsafe {
// Traditionally there was a difference between reading data or
Expand All @@ -176,6 +177,7 @@ pub fn read(pid: Pid, addr: AddressType) -> Result<c_int> {
}

/// Writes a word into the processes memory at the given address
#[allow(clippy::not_unsafe_ptr_arg_deref)]
pub fn write(pid: Pid, addr: AddressType, data: c_int) -> Result<()> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ptrace::write is unsafe on Linux. Rather than suppress this warning, do we want to just make the ptrace API consistently unsafe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically this doesn't violate Rust's safety rules. libc::ptrace doesn't dereference those addresses. Instead, it passes them to the kernel as-is. So from a safety perspective, this is similar to a Rust function that accepts a raw pointer argument but does nothing more than print its debugging form.
OTOH, you could make the argument that since any foreign function call is unsafe, and we don't really know what ptrace does, that the entire ptrace API ought to be unsafe.
I usually hew to the narrow technical definition of unsafe, but in this case I think making the entire API unsafe would be sensible. It would probably match users' expectations.

unsafe { ptrace_other(Request::PT_WRITE_D, pid, addr, data).map(drop) }
}