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

Add as_ptr_cast_mut lint #9572

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Add as_ptr_cast_mut lint #9572

merged 3 commits into from
Oct 11, 2022

Conversation

Noratrieb
Copy link
Member

This lint detects calls to a &self-taking as_ptr method, where the result is then immediately cast to a *mut T. Code like this is probably invalid, as that pointer will not have write permissions, and *mut T is usually used to write through.

Examples of broken code with this pattern:
https://miri.saethlin.dev/ub?crate=lol_alloc&version=0.1.3
https://miri.saethlin.dev/ub?crate=sophon-wasm&version=0.19.0
https://miri.saethlin.dev/ub?crate=polars-core&version=0.24.2
https://miri.saethlin.dev/ub?crate=ach-cell&version=0.1.17

changelog: Add [as_ptr_cast_mut]

@rust-highfive
Copy link

r? @dswij

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 1, 2022
This lint detects calls to a `&self`-taking `as_ptr` method, where
the result is then immediately cast to a `*mut T`. Code like this
is probably invalid, as that pointer will not have write permissions,
and `*mut T` is usually used to write through.
/// Checks for the result of a `&self`-taking `as_ptr` being cast to a mutable pointer
///
/// ### Why is this bad?
/// Since `as_ptr` took a `&self`, the pointer won't have write permissions, making it
Copy link
Member

Choose a reason for hiding this comment

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

Won't is a bit too strong. This is not a dangerous pattern if the pointer points into an UnsafeCell.

I don't know how technically correct or detailed clippy docs are expected to be, but this should not be so absolute.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a unless interior mutability is used which makes this correct, but it doesn't sound great in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps

    /// Since `as_ptr` takes a `&self`, the pointer won't have write permissions unless interior
    /// mutability is used. If this cast to `*mut` exists to enable mutation through this pointer,
    /// that mutation is likely to be UB.

Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

This LGTM. I think the wording is ok now. If the wording is indeed too confusing, we can improve it in the future.

Comment on lines +25 to +26
// `as_mut_ptr` might not exist
let applicability = Applicability::MaybeIncorrect;
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 we can check if it implements as_mut_ptr for the suggestion, but this can be a future improvement

@dswij
Copy link
Member

dswij commented Oct 11, 2022

Thanks for this @Nilstrieb!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 11, 2022

📌 Commit 2b944d0 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 11, 2022

⌛ Testing commit 2b944d0 with merge 8e87d39...

@bors
Copy link
Collaborator

bors commented Oct 11, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 8e87d39 to master...

@bors bors merged commit 8e87d39 into rust-lang:master Oct 11, 2022
@Noratrieb Noratrieb deleted the as-ptr-cast-mut branch October 11, 2022 08:39
@bors bors mentioned this pull request Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants