diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a9be0214372..c00f4462e17e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6533,6 +6533,7 @@ Released 2018-09-13 [`range_plus_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one [`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero [`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len +[`raw_assign_to_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#raw_assign_to_drop [`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer [`rc_clone_in_vec_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_clone_in_vec_init [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 1b19a8851edd..93fd725a92aa 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -599,6 +599,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::operators::MODULO_ONE_INFO, crate::operators::NEEDLESS_BITWISE_BOOL_INFO, crate::operators::OP_REF_INFO, + crate::operators::RAW_ASSIGN_TO_DROP_INFO, crate::operators::REDUNDANT_COMPARISONS_INFO, crate::operators::SELF_ASSIGNMENT_INFO, crate::operators::VERBOSE_BIT_MASK_INFO, diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index bdbbb3475cd5..2cb29684c411 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -19,6 +19,7 @@ mod modulo_one; mod needless_bitwise_bool; mod numeric_arithmetic; mod op_ref; +mod raw_assign_to_drop; mod self_assignment; mod verbose_bit_mask; @@ -807,6 +808,67 @@ declare_clippy_lint! { "explicit self-assignment" } +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for assignments via raw pointers that involve types with destructors. + /// + /// ### Why restrict this? + /// + /// Assignments of the form `*ptr = new_value;` assume that `*ptr` contains an initialized + /// value, and unconditionally execute the `std::ops::Drop`-implementation if such + /// implementation is defined on the type of `*ptr`. If the old value is in fact + /// uninitialized or otherwise invalid, the execution of `std::ops::Drop::drop(&mut self)` + /// is always Undefined Behavior. + /// + /// The unsafe-block required to assign via the raw-pointer should include a SAFETY-comment + /// that explains why it is always safe to execute the destructor. + /// + /// If the code can not guarantee that the old value can be safely dropped, use + /// `std::ptr::write()` to overwrite the (possibly nonexisting) value without executing + /// the destructor; this may leak resources if the old value did in fact exist. + /// + /// Use `std::ptr::drop_in_place()` to (selectively) execute the destructor, having + /// established that it is safe to do so. + /// + /// ### Example + /// ```no_run + /// unsafe fn foo(oldvalue: *mut String) { + /// // Direct assignment always executes `String`'s destructor on `oldvalue`. + /// // However, we can't guarantee that executing the destructor is safe. + /// unsafe { *oldvalue = "New Value".to_owned(); } + /// } + /// ``` + /// Use instead: + /// ```no_run + /// #[allow(clippy::raw_assign_to_drop)] + /// unsafe fn foo(oldvalue: *mut String, oldvalue_is_initialized: bool) { + /// let newvalue = "New Value".to_owned(); + /// + /// // In this example, `oldvalue_is_initialized` is an oracle for what + /// // the programmer might be able to establish statically. + /// if oldvalue_is_initialized { + /// // Having established that `oldvalue` points to a valid value, it is safe to + /// // execute the destructor and assign a new value. + /// + /// // SAFETY: oldvalue is properly aligned, valid for writes, and initialized + /// unsafe { + /// *oldvalue = newvalue; + /// } + /// } else { + /// // Overwrite the old value without running the destructor. + /// + /// // SAFETY: oldvalue is properly aligned and valid for writes + /// unsafe { oldvalue.write(newvalue); } + /// } + /// } + /// ``` + #[clippy::version = "1.91.0"] + pub RAW_ASSIGN_TO_DROP, + restriction, + "assignment via raw pointer that involves destructors" +} + declare_clippy_lint! { /// ### What it does /// Checks for manual implementation of `midpoint`. @@ -904,6 +966,7 @@ impl_lint_pass!(Operators => [ MODULO_ARITHMETIC, NEEDLESS_BITWISE_BOOL, SELF_ASSIGNMENT, + RAW_ASSIGN_TO_DROP, MANUAL_MIDPOINT, MANUAL_IS_MULTIPLE_OF, ]); @@ -954,6 +1017,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { ExprKind::Assign(lhs, rhs, _) => { assign_op_pattern::check(cx, e, lhs, rhs, self.msrv); self_assignment::check(cx, e, lhs, rhs); + raw_assign_to_drop::check(cx, lhs); }, ExprKind::Unary(op, arg) => { if op == UnOp::Neg { diff --git a/clippy_lints/src/operators/raw_assign_to_drop.rs b/clippy_lints/src/operators/raw_assign_to_drop.rs new file mode 100644 index 000000000000..abe734671a4c --- /dev/null +++ b/clippy_lints/src/operators/raw_assign_to_drop.rs @@ -0,0 +1,43 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use rustc_hir::{Expr, ExprKind, UnOp}; +use rustc_lint::LateContext; + +use super::RAW_ASSIGN_TO_DROP; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, lhs: &'tcx Expr<'_>) { + if let ExprKind::Unary(UnOp::Deref, expr) = lhs.kind + && let ty = cx.typeck_results().expr_ty(expr) + && ty.is_raw_ptr() + && let Some(deref_ty) = ty.builtin_deref(true) + && deref_ty.needs_drop(cx.tcx, cx.typing_env()) + { + if let ExprKind::MethodCall(path, self_arg, [], ..) = expr.kind + && let rustc_middle::ty::Adt(ty_def, ..) = cx.typeck_results().expr_ty(self_arg).kind() + && ty_def.is_unsafe_cell() + && path.ident.as_str() == "get" + { + // Don't lint if the raw pointer was directly retrieved from UnsafeCell::get() + // We assume those to be safely managed + return; + } + span_lint_and_then( + cx, + RAW_ASSIGN_TO_DROP, + expr.span, + "assignment via raw pointer always executes destructor", + |diag| { + diag.note(format!( + "the destructor defined by `{deref_ty}` is executed during assignment of the new value" + )); + diag.span_label( + expr.span, + "the old value may be uninitialized, causing Undefined Behavior when the destructor executes", + ); + diag.help("use `std::ptr::write()` to overwrite a possibly uninitialized place"); + diag.help( + "use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists", + ); + }, + ); + } +} diff --git a/tests/ui/raw_assign_to_drop.rs b/tests/ui/raw_assign_to_drop.rs new file mode 100644 index 000000000000..8970b0ac5a09 --- /dev/null +++ b/tests/ui/raw_assign_to_drop.rs @@ -0,0 +1,42 @@ +#![warn(clippy::raw_assign_to_drop)] +#![allow(clippy::missing_safety_doc)] + +use std::cell::UnsafeCell; + +pub unsafe fn foo(r: *mut String, i: *mut i32) { + unsafe { + *r = "foo".to_owned(); + //~^ raw_assign_to_drop + + // no lint on {integer} + *i = 47; + + (*r, *r) = ("foo".to_owned(), "bar".to_owned()); + //~^ raw_assign_to_drop + //~^^ raw_assign_to_drop + + (*r, *i) = ("foo".to_owned(), 47); + //~^ raw_assign_to_drop + + let mut x: String = Default::default(); + *(&mut x as *mut _) = "Foo".to_owned(); + //~^ raw_assign_to_drop + + // no lint on `u8` + *x.as_mut_ptr() = b'a'; + + let mut v: Vec = vec![]; + *v.as_mut_ptr() = Default::default(); + //~^ raw_assign_to_drop + } +} + +pub unsafe fn unsafecell() { + // No lint + let c = UnsafeCell::new(String::new()); + unsafe { + *c.get() = String::new(); + } +} + +fn main() {} diff --git a/tests/ui/raw_assign_to_drop.stderr b/tests/ui/raw_assign_to_drop.stderr new file mode 100644 index 000000000000..ec0fe7e896ea --- /dev/null +++ b/tests/ui/raw_assign_to_drop.stderr @@ -0,0 +1,64 @@ +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:8:10 + | +LL | *r = "foo".to_owned(); + | ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes + | + = note: the destructor defined by `std::string::String` is executed during assignment of the new value + = help: use `std::ptr::write()` to overwrite a possibly uninitialized place + = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists + = note: `-D clippy::raw-assign-to-drop` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::raw_assign_to_drop)]` + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:14:11 + | +LL | (*r, *r) = ("foo".to_owned(), "bar".to_owned()); + | ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes + | + = note: the destructor defined by `std::string::String` is executed during assignment of the new value + = help: use `std::ptr::write()` to overwrite a possibly uninitialized place + = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:14:15 + | +LL | (*r, *r) = ("foo".to_owned(), "bar".to_owned()); + | ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes + | + = note: the destructor defined by `std::string::String` is executed during assignment of the new value + = help: use `std::ptr::write()` to overwrite a possibly uninitialized place + = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:18:11 + | +LL | (*r, *i) = ("foo".to_owned(), 47); + | ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes + | + = note: the destructor defined by `std::string::String` is executed during assignment of the new value + = help: use `std::ptr::write()` to overwrite a possibly uninitialized place + = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:22:10 + | +LL | *(&mut x as *mut _) = "Foo".to_owned(); + | ^^^^^^^^^^^^^^^^^^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes + | + = note: the destructor defined by `std::string::String` is executed during assignment of the new value + = help: use `std::ptr::write()` to overwrite a possibly uninitialized place + = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:29:10 + | +LL | *v.as_mut_ptr() = Default::default(); + | ^^^^^^^^^^^^^^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes + | + = note: the destructor defined by `std::string::String` is executed during assignment of the new value + = help: use `std::ptr::write()` to overwrite a possibly uninitialized place + = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists + +error: aborting due to 6 previous errors +