diff --git a/CHANGELOG.md b/CHANGELOG.md index ef6140152ff..754d5f506d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3735,6 +3735,7 @@ Released 2018-09-13 [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant [`arithmetic_side_effects`]: https://rust-lang.github.io/rust-clippy/master/index.html#arithmetic_side_effects [`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions +[`as_ptr_cast_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut [`as_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore [`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants [`assertions_on_result_states`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_result_states diff --git a/clippy_lints/src/casts/as_ptr_cast_mut.rs b/clippy_lints/src/casts/as_ptr_cast_mut.rs new file mode 100644 index 00000000000..9409f4844f5 --- /dev/null +++ b/clippy_lints/src/casts/as_ptr_cast_mut.rs @@ -0,0 +1,38 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_opt; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::{ + mir::Mutability, + ty::{self, Ty, TypeAndMut}, +}; + +use super::AS_PTR_CAST_MUT; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_expr: &Expr<'_>, cast_to: Ty<'_>) { + if let ty::RawPtr(ptrty @ TypeAndMut { mutbl: Mutability::Mut, .. }) = cast_to.kind() + && let ty::RawPtr(TypeAndMut { mutbl: Mutability::Not, .. }) = + cx.typeck_results().node_type(cast_expr.hir_id).kind() + && let ExprKind::MethodCall(method_name, receiver, [], _) = cast_expr.peel_blocks().kind + && method_name.ident.name == rustc_span::sym::as_ptr + && let Some(as_ptr_did) = cx.typeck_results().type_dependent_def_id(cast_expr.peel_blocks().hir_id) + && let as_ptr_sig = cx.tcx.fn_sig(as_ptr_did) + && let Some(first_param_ty) = as_ptr_sig.skip_binder().inputs().iter().next() + && let ty::Ref(_, _, Mutability::Not) = first_param_ty.kind() + && let Some(recv) = snippet_opt(cx, receiver.span) + { + // `as_mut_ptr` might not exist + let applicability = Applicability::MaybeIncorrect; + + span_lint_and_sugg( + cx, + AS_PTR_CAST_MUT, + expr.span, + &format!("casting the result of `as_ptr` to *{ptrty}"), + "replace with", + format!("{recv}.as_mut_ptr()"), + applicability + ); + } +} diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index cc5d346b954..fb0bd4d30f7 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -1,3 +1,4 @@ +mod as_ptr_cast_mut; mod as_underscore; mod borrow_as_ptr; mod cast_abs_to_unsigned; @@ -596,6 +597,32 @@ declare_clippy_lint! { "casting a slice created from a pointer and length to a slice pointer" } +declare_clippy_lint! { + /// ### What it does + /// 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 + /// unlikely that having it as a mutable pointer is correct. + /// + /// ### Example + /// ```rust + /// let string = String::with_capacity(1); + /// let ptr = string.as_ptr() as *mut _; + /// unsafe { ptr.write(4) }; // UNDEFINED BEHAVIOUR + /// ``` + /// Use instead: + /// ```rust + /// let mut string = String::with_capacity(1); + /// let string = string.as_mut_ptr(); + /// unsafe { ptr.write(4) }; + /// ``` + #[clippy::version = "1.66.0"] + pub AS_PTR_CAST_MUT, + nursery, + "casting the result of the `&self`-taking as_ptr to a mutabe point" +} + pub struct Casts { msrv: Option, } @@ -627,7 +654,8 @@ impl_lint_pass!(Casts => [ CAST_ABS_TO_UNSIGNED, AS_UNDERSCORE, BORROW_AS_PTR, - CAST_SLICE_FROM_RAW_PARTS + CAST_SLICE_FROM_RAW_PARTS, + AS_PTR_CAST_MUT, ]); impl<'tcx> LateLintPass<'tcx> for Casts { @@ -653,6 +681,7 @@ impl<'tcx> LateLintPass<'tcx> for Casts { return; } cast_slice_from_raw_parts::check(cx, expr, cast_expr, cast_to, self.msrv); + as_ptr_cast_mut::check(cx, expr, cast_expr, cast_to); fn_to_numeric_cast_any::check(cx, expr, cast_expr, cast_from, cast_to); fn_to_numeric_cast::check(cx, expr, cast_expr, cast_from, cast_to); fn_to_numeric_cast_with_truncation::check(cx, expr, cast_expr, cast_from, cast_to); diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index ee08d802ccf..2c849701365 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -66,6 +66,7 @@ store.register_lints(&[ cargo::NEGATIVE_FEATURE_NAMES, cargo::REDUNDANT_FEATURE_NAMES, cargo::WILDCARD_DEPENDENCIES, + casts::AS_PTR_CAST_MUT, casts::AS_UNDERSCORE, casts::BORROW_AS_PTR, casts::CAST_ABS_TO_UNSIGNED, diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index 87be0052028..a75bc81b222 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/clippy_lints/src/lib.register_nursery.rs @@ -4,6 +4,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(attrs::EMPTY_LINE_AFTER_OUTER_ATTR), + LintId::of(casts::AS_PTR_CAST_MUT), LintId::of(cognitive_complexity::COGNITIVE_COMPLEXITY), LintId::of(copies::BRANCHES_SHARING_CODE), LintId::of(derive::DERIVE_PARTIAL_EQ_WITHOUT_EQ), diff --git a/src/docs.rs b/src/docs.rs index 166be0618ff..c3cbd2942d8 100644 --- a/src/docs.rs +++ b/src/docs.rs @@ -28,6 +28,7 @@ docs! { "approx_constant", "arithmetic_side_effects", "as_conversions", + "as_ptr_cast_mut", "as_underscore", "assertions_on_constants", "assertions_on_result_states", diff --git a/src/docs/as_ptr_cast_mut.txt b/src/docs/as_ptr_cast_mut.txt new file mode 100644 index 00000000000..0192a0b686f --- /dev/null +++ b/src/docs/as_ptr_cast_mut.txt @@ -0,0 +1,19 @@ +### What it does +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 +unlikely that having it as a mutable pointer is correct. + +### Example +``` +let string = String::with_capacity(1); +let ptr = string.as_ptr() as *mut _; +unsafe { ptr.write(4) }; // UNDEFINED BEHAVIOUR +``` +Use instead: +``` +let mut string = String::with_capacity(1); +let string = string.as_mut_ptr(); +unsafe { ptr.write(4) }; +``` \ No newline at end of file diff --git a/tests/ui/as_ptr_cast_mut.rs b/tests/ui/as_ptr_cast_mut.rs new file mode 100644 index 00000000000..0d1d9258433 --- /dev/null +++ b/tests/ui/as_ptr_cast_mut.rs @@ -0,0 +1,37 @@ +#![allow(unused)] +#![warn(clippy::as_ptr_cast_mut)] +#![allow(clippy::wrong_self_convention)] + +struct MutPtrWrapper(Vec); +impl MutPtrWrapper { + fn as_ptr(&mut self) -> *const u8 { + self.0.as_mut_ptr() as *const u8 + } +} + +struct Covariant(*const T); +impl Covariant { + fn as_ptr(self) -> *const T { + self.0 + } +} + +fn main() { + let mut string = String::new(); + let _ = string.as_ptr() as *mut u8; + let _: *mut i8 = string.as_ptr() as *mut _; + let _ = string.as_ptr() as *const i8; + let _ = string.as_mut_ptr(); + let _ = string.as_mut_ptr() as *mut u8; + let _ = string.as_mut_ptr() as *const u8; + + let nn = std::ptr::NonNull::new(4 as *mut u8).unwrap(); + let _ = nn.as_ptr() as *mut u8; + + let mut wrap = MutPtrWrapper(Vec::new()); + let _ = wrap.as_ptr() as *mut u8; + + let mut local = 4; + let ref_with_write_perm = Covariant(std::ptr::addr_of_mut!(local) as *const _); + let _ = ref_with_write_perm.as_ptr() as *mut u8; +} diff --git a/tests/ui/as_ptr_cast_mut.stderr b/tests/ui/as_ptr_cast_mut.stderr new file mode 100644 index 00000000000..2189c3d2f85 --- /dev/null +++ b/tests/ui/as_ptr_cast_mut.stderr @@ -0,0 +1,16 @@ +error: casting the result of `as_ptr` to *mut u8 + --> $DIR/as_ptr_cast_mut.rs:21:13 + | +LL | let _ = string.as_ptr() as *mut u8; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `string.as_mut_ptr()` + | + = note: `-D clippy::as-ptr-cast-mut` implied by `-D warnings` + +error: casting the result of `as_ptr` to *mut i8 + --> $DIR/as_ptr_cast_mut.rs:22:22 + | +LL | let _: *mut i8 = string.as_ptr() as *mut _; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `string.as_mut_ptr()` + +error: aborting due to 2 previous errors +