diff --git a/CHANGELOG.md b/CHANGELOG.md index b6033de93501..1c1b963b955a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5930,6 +5930,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 7451fb909ef8..c03e71288dc0 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -599,6 +599,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::operators::NEEDLESS_BITWISE_BOOL_INFO, crate::operators::OP_REF_INFO, crate::operators::PTR_EQ_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 9e8a821c3f4e..738cca04ad53 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -18,6 +18,7 @@ mod needless_bitwise_bool; mod numeric_arithmetic; mod op_ref; mod ptr_eq; +mod raw_assign_to_drop; mod self_assignment; mod verbose_bit_mask; @@ -837,6 +838,49 @@ declare_clippy_lint! { "explicit self-assignment" } +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for assignments via raw pointers that involve types with destructors. + /// + /// ### Why is this bad? + /// + /// 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 value is in fact + /// uninitialized or otherwise invalid, the execution of `std::ops::Drop::drop(&mut self)` + /// is always Undefined Behavior. + /// + /// Use `std::ptr::write()` to overwrite a value without executing the destructor. + /// + /// Use `std::ptr::drop_in_place()` to conditionally execute the destructor if you are + /// sure that the place contains an initialized value. + /// + /// ### Example + /// ```no_run + /// unsafe fn foo(oldvalue: *mut String) { + /// // Direct assignment always executes `String`'s destructor on `oldvalue` + /// *oldvalue = "New Value".to_owned(); + /// } + /// ``` + /// Use instead: + /// ```no_run + /// unsafe fn foo(oldvalue: *mut String, oldvalue_is_initialized: bool) { + /// if oldvalue_is_initialized { + /// // Having established that `oldvalue` points to a valid value, selectively + /// // execute the destructor to prevent a memory-leak + /// oldvalue.drop_in_place(); + /// } + /// // Overwrite the old value without running the destructor unconditionally + /// oldvalue.write("New Value".to_owned()); + /// } + /// ``` + #[clippy::version = "1.85.0"] + pub RAW_ASSIGN_TO_DROP, + suspicious, + "assignment via raw pointer that involves destructors" +} + pub struct Operators { arithmetic_context: numeric_arithmetic::Context, verbose_bit_mask_threshold: u64, @@ -879,6 +923,7 @@ impl_lint_pass!(Operators => [ NEEDLESS_BITWISE_BOOL, PTR_EQ, SELF_ASSIGNMENT, + RAW_ASSIGN_TO_DROP, ]); impl<'tcx> LateLintPass<'tcx> for Operators { @@ -925,6 +970,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { ExprKind::Assign(lhs, rhs, _) => { assign_op_pattern::check(cx, e, lhs, rhs); 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..be0b2969b5ea --- /dev/null +++ b/clippy_lints/src/operators/raw_assign_to_drop.rs @@ -0,0 +1,32 @@ +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_unsafe_ptr() + && let Some(deref_ty) = ty.builtin_deref(true) + && deref_ty.needs_drop(cx.tcx, cx.typing_env()) + { + 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, + "this place 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 if 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..30d9cc8025d5 --- /dev/null +++ b/tests/ui/raw_assign_to_drop.rs @@ -0,0 +1,23 @@ +#![warn(clippy::raw_assign_to_drop)] + +fn main() { + unsafe fn foo(r: *mut String, i: *mut i32) { + *r = "foo".to_owned(); + + // no lint on {integer} + *i = 47; + + (*r, *r) = ("foo".to_owned(), "bar".to_owned()); + + (*r, *i) = ("foo".to_owned(), 47); + + let mut x: String = Default::default(); + *(&mut x as *mut _) = "Foo".to_owned(); + + // no lint on `u8` + *x.as_mut_ptr() = b'a'; + + let mut v: Vec = vec![]; + *v.as_mut_ptr() = Default::default(); + } +} diff --git a/tests/ui/raw_assign_to_drop.stderr b/tests/ui/raw_assign_to_drop.stderr new file mode 100644 index 000000000000..ed1be26cd985 --- /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:5:10 + | +LL | *r = "foo".to_owned(); + | ^ this place 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 if 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:10:11 + | +LL | (*r, *r) = ("foo".to_owned(), "bar".to_owned()); + | ^ this place 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 if such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:10:15 + | +LL | (*r, *r) = ("foo".to_owned(), "bar".to_owned()); + | ^ this place 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 if such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:12:11 + | +LL | (*r, *i) = ("foo".to_owned(), 47); + | ^ this place 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 if such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:15:10 + | +LL | *(&mut x as *mut _) = "Foo".to_owned(); + | ^^^^^^^^^^^^^^^^^^ this place 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 if such value exists + +error: assignment via raw pointer always executes destructor + --> tests/ui/raw_assign_to_drop.rs:21:10 + | +LL | *v.as_mut_ptr() = Default::default(); + | ^^^^^^^^^^^^^^ this place 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 if such value exists + +error: aborting due to 6 previous errors +