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.

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

not_unsafe_ptr_arg_deref false positives #3045

Open
Tracked by #79
gnzlbg opened this issue Aug 13, 2018 · 14 comments
Open
Tracked by #79

not_unsafe_ptr_arg_deref false positives #3045

gnzlbg opened this issue Aug 13, 2018 · 14 comments
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 13, 2018

Playground:

pub struct X2<T>(T, T);

impl<T> X2<*mut T> {
    pub fn foo(self, a: *mut T) -> Self {
        unsafe { self.bar(a) }
    }
    
    pub unsafe fn bar(self, _a: *mut T) -> Self {
        self
    }
}
fn main() {}

produces

error: this public function dereferences a raw pointer but is not marked `unsafe`
 --> src/main.rs:5:27
  |
5 |         unsafe { self.bar(a) }
  |                           ^
  |
  = note: #[deny(not_unsafe_ptr_arg_deref)] on by default
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#not_unsafe_ptr_arg_deref

which is incorrect since no raw pointer is ever dereferenced anywhere.

@oli-obk oli-obk added the C-bug Category: Clippy is not doing the correct thing label Aug 13, 2018
@lucab
Copy link
Contributor

lucab commented Sep 20, 2018

Hit the same just passing-through a pointer to an FFI extern function:

use std::os::raw::{c_int, c_void};

extern "C" {
    pub fn libfoo_set_item(h: c_int, t: c_int, item: *const c_void) -> c_int;
}

pub fn set_item(_h: (), _t: (), item: *const c_void) {
    unsafe { libfoo_set_item(0, 0, item) };
}

fn main() {}

@ghost
Copy link

ghost commented Sep 21, 2018

I don't get this lint. As I understand it, you mark a function as unsafe when it's the callers job to check prerequisites and indicate that it can have undefined behavior if they aren't met. You don't use unsafe if you're doing the checks and ensuring there is no undefined behavior within the function. Whether you're calling unsafe functions or derefencing raw pointers has nothing to do with it. Am I wrong?

@Manishearth
Copy link
Member

Bear in mind that safe code can synthesize raw pointers at will.

A function accepting a raw pointer that it uses can almost never be safe. How do you check pointer validity? It could be a pointer to anything. It's not just null checks you have to do. You're right that it is the responsibility of safe functions to check invariants -- but that's basically almost impossible to do in this case.

There is one exception: if you actually maintain a list of valid pointers and validate against that. I've only seen this pattern used once, it's not common enough to attempt to catch.

Not using the pointer is definitely a false positive (albeit a bit weird)

The FFI example is not a false positive.

This is a pretty important lint -- this kind of mistake (misjudging boundaries) is not uncommon in unsafe code

@lucab
Copy link
Contributor

lucab commented Sep 21, 2018

The FFI example is not a false positive.

In the FFI example, the Rust code as well does not dereference the pointer in set_item, it is only passed through (to the best of my understanding). The native libfoo_set_item may internally do that, but that's outside of Rust scope and it is not actionable for this lint.

Which is why, unless I badly misunderstood something, I think I'm hitting a variation of this false positive.

@Manishearth
Copy link
Member

The only thing that the FFI function can do is :

  • never use it
  • check against a list of valid pointers
  • cast to an integer and do something with that

With (a) ... why do you need the argument? (b) is rare, and (c) should have a usize API.

You're right that C is outside the scope of Rust, but this is Clippy, not rustc, and Clippy knows that it is almost impossible for this to be safe and is in its rights to complain.

It is definitely actionable in the lint -- make the function unsafe, or make it accept a pointer abstraction that is unsafe to synthesize.

@lucab
Copy link
Contributor

lucab commented Sep 21, 2018

Neither libfoo_set_item nor item: *const c_void are under my (i.e. Rust code) control here. Those pointers are coming from another part of the native libfoo and the Rust wrappers are just opaquely passing them around, and carefully trying not to directly access them. You have good valid points on C API design and practices, but this is not relevant when linking to external third-party libraries (i.e. not self-developed and usually part of historical standard interface).

While I can hide pointers in some newtype or mark all the Rust functions unsafe, we still didn't move from the initial fact that the snippet above does not do any pointer dereference in the Rust code (and the native code function doing that is already extern/unsafe).

@Manishearth
Copy link
Member

In that case it is definitely wrong to have a safe API that takes a raw pointer and blindly hand it over to C code.

My API design argument was not my main argument here, you're missing the point. If a C API is accepting a pointer it is very likely that it intends to eventually dereference it, at most with a null check. There isn't more checking it can do. You cannot give safe rust code the ability to pass down arbitrary pointer values to it.

This is enough to justify the lint.

The API design point was dealing with the edge cases -- when the C API accepts a pointer without intent to dereference it. These are reasonably rare, I just gave a counterargument for completeness' sake.

The fact that the rust code is not doing the pointer dereference isn't relevant, it's very likely that someone is, and it's still the Rust code's responsibility to maintain the safety invariants, but we know can't. This is a very fundamental thing about writing unsafe code, you have to make sure you take responsibility for the invariants when you mark an unsafety-containung function as safe.

Simply put, in safe code, I can call, set_item((),(), 0x123456000 as *const _) and it's most likely going to get dereferenced somewhere down the way. There's not much in the way of safety checks that can be done here. Clippy knows this, and complains about it.

@lucab
Copy link
Contributor

lucab commented Sep 21, 2018

@Manishearth thanks for the followup. I definitely see your point (and in the previous comment too, I was trying to bring this down to a concrete real-word scenario).

@stevenengler
Copy link
Contributor

If this is the expected behaviour, I think the message should be changed. Maybe something like "this public function might dereference a raw pointer but is not marked `unsafe`"? The current message is confusing and not always correct.

@Manishearth
Copy link
Member

Sure, I'd accept that change

bors added a commit that referenced this issue May 31, 2021
Improve message for `not_unsafe_ptr_arg_deref` lint

changelog: Improved message for the ['not_unsafe_ptr_arg_deref'] lint

Doesn't close any issue, but implements a suggestion from #3045 (comment).
@jmeggitt
Copy link

jmeggitt commented Apr 9, 2022

Hey, I just wanted to add another example of a false positive that I just ran into that does not involve FFI. I was in the process of fixing some old Rust code which looked something like this when I encountered not_unsafe_ptr_arg_deref. Playground:

#[repr(transparent)]
pub struct Foo(usize);

impl Foo {
    pub fn from_ptr<T>(ptr: *const T) -> Self {
        unsafe { mem::transmute(ptr) }
    }
}

Now there are a lot of things wrong with this code (Personally I don't think Foo should even exist in the first place), but I felt that this clippy error caused a lot more confusion than help.

error: this public function might dereference a raw pointer but is not marked `unsafe`
  --> src\common\mod.rs:74:33
   |
74 |         unsafe { mem::transmute(ptr) }
   |                                 ^^^
   |
   = note: `#[deny(clippy::not_unsafe_ptr_arg_deref)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref

My initial reaction upon seeing this error was that Clippy was broken since there is no way std::mem::transmute would ever dereference a raw pointer and it has already been marked unsafe. One way to make this more helpful would be to highlight both the function that should be unsafe and the location that might dereference a pointer.

That being said, it seems likely that std::mem::transmute is a special edge case that is not properly being handled. I propose any transmute of a raw-pointer to a non-pointer type result in a wrong_transmute lint. A "non-pointer type" may seem a bit ambiguous, but it can be narrowed down to any types other than raw pointers and function pointers which would not trigger the linter for crosspointer_transmute, transmute_ptr_to_ref, or transmutes_expressible_as_ptr_casts.

@crazyboycjr
Copy link

crazyboycjr commented Oct 21, 2022

I'm providing another false positive, in core::ptr::NonNull<T>.

#![feature(const_ptr_is_null)]

#[repr(transparent)]
pub struct NonNull<T: ?Sized> {
    pointer: *const T,
}

impl<T: ?Sized> NonNull<T> {
    #[inline]
    pub const unsafe fn new_unchecked(ptr: *mut T) -> Self {
        // SAFETY: the caller must guarantee that `ptr` is non-null.
        unsafe { NonNull { pointer: ptr as _ } }
    }

    #[inline]
    pub const fn new(ptr: *mut T) -> Option<Self> {
        if !ptr.is_null() {
            // SAFETY: The pointer is already checked and is not null
            Some(unsafe { Self::new_unchecked(ptr) })
        } else {
            None
        }
    }
}

fn main() { }

cargo clippy produces

error: this public function might dereference a raw pointer but is not marked `unsafe`
  --> src/main.rs:19:47
   |
19 |             Some(unsafe { Self::new_unchecked(ptr) })
   |                                               ^^^
   |
   = note: `#[deny(clippy::not_unsafe_ptr_arg_deref)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref

@pravic
Copy link

pravic commented Feb 20, 2023

@Manishearth Regarding your arguments:

With the false positive above even in NonNull (the core type!) it's clear that currently it is not possible to have a safe wrapper that satisfies clippy::not_unsafe_ptr_arg_deref. Is there any solution (for NonNull and similar wrappers) besides disabling the lint?

@Manishearth
Copy link
Member

Manishearth commented Feb 20, 2023

Yes, and "pointer wrapper types" are rather rare. I would consider such types as a valid reason to disable the lint for the module implementing these types. Clippy lints are by-design not supposed to be perfect, they will need to be disabled based on user preferences or specific edge cases1, this is a case where you acknowledge the lint and disable it.

We could potentially allowlist a bunch of APIs and actually check the body of such functions, it's tricky but doable and would cut down on false positives.

Footnotes

  1. Were that not true for a lint, it would belong in rustc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

No branches or pull requests

8 participants