diff --git a/CHANGELOG.md b/CHANGELOG.md index dbdf3df4ddc3..1865c39c664b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1050,6 +1050,7 @@ Released 2018-09-13 [`mem_discriminant_non_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum [`mem_forget`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_forget [`mem_replace_option_with_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_none +[`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit [`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max [`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op diff --git a/README.md b/README.md index dd315fd397b0..4541af9c844e 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 313 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 314 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9d0a91c53182..abd314c6be5e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -781,6 +781,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con matches::SINGLE_MATCH, mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, mem_replace::MEM_REPLACE_OPTION_WITH_NONE, + mem_replace::MEM_REPLACE_WITH_UNINIT, methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, methods::CLONE_DOUBLE_REF, @@ -1116,6 +1117,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con loops::REVERSE_RANGE_LOOP, loops::WHILE_IMMUTABLE_CONDITION, mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, + mem_replace::MEM_REPLACE_WITH_UNINIT, methods::CLONE_DOUBLE_REF, methods::INTO_ITER_ON_ARRAY, methods::TEMPORARY_CSTRING_AS_PTR, diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index 7e83e836b856..2bde5ebd72e3 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -1,4 +1,5 @@ -use crate::utils::{match_def_path, match_qpath, paths, snippet_with_applicability, span_lint_and_sugg}; +use crate::utils::{match_def_path, match_qpath, paths, snippet_with_applicability, + span_help_and_lint, span_lint_and_sugg}; use if_chain::if_chain; use rustc::hir::{Expr, ExprKind, MutMutable, QPath}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; @@ -32,7 +33,39 @@ declare_clippy_lint! { "replacing an `Option` with `None` instead of `take()`" } -declare_lint_pass!(MemReplace => [MEM_REPLACE_OPTION_WITH_NONE]); +declare_clippy_lint! { + /// **What it does:** Checks for `mem::replace(&mut _, mem::uninitialized())` + /// and `mem::replace(&mut _, mem::zeroed())`. + /// + /// **Why is this bad?** This will lead to undefined behavior even if the + /// value is overwritten later, because the uninitialized value may be + /// observed in the case of a panic. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ``` + /// use std::mem; + ///# fn may_panic(v: Vec) -> Vec { v } + /// + /// fn myfunc (v: &mut Vec) { + /// let taken_v = unsafe { mem::replace(&mut v, mem::uninitialized()) }; + /// let new_v = may_panic(taken_v); // undefined behavior on panic + /// mem::forget(mem::replace(&mut v, new_v)); + /// } + /// ``` + /// + /// The [take_mut](https://docs.rs/take_mut) crate offers a sound solution, + /// at the cost of aborting on panic, to ensure that the uninitialized + /// value cannot be observed. + pub MEM_REPLACE_WITH_UNINIT, + correctness, + "`mem::replace(&mut _, mem::uninitialized())` or `mem::zeroed()`" +} + +declare_lint_pass!(MemReplace => + [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemReplace { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { @@ -46,36 +79,47 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemReplace { // Check that second argument is `Option::None` if let ExprKind::Path(ref replacement_qpath) = func_args[1].node; - if match_qpath(replacement_qpath, &paths::OPTION_NONE); - then { - // Since this is a late pass (already type-checked), - // and we already know that the second argument is an - // `Option`, we do not need to check the first - // argument's type. All that's left is to get - // replacee's path. - let replaced_path = match func_args[0].node { - ExprKind::AddrOf(MutMutable, ref replaced) => { - if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.node { - replaced_path - } else { - return - } - }, - ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path, - _ => return, - }; + if match_qpath(replacement_qpath, &paths::OPTION_NONE) { + + // Since this is a late pass (already type-checked), + // and we already know that the second argument is an + // `Option`, we do not need to check the first + // argument's type. All that's left is to get + // replacee's path. + let replaced_path = match func_args[0].node { + ExprKind::AddrOf(MutMutable, ref replaced) => { + if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.node { + replaced_path + } else { + return + } + }, + ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path, + _ => return, + }; - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - MEM_REPLACE_OPTION_WITH_NONE, - expr.span, - "replacing an `Option` with `None`", - "consider `Option::take()` instead", - format!("{}.take()", snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)), - applicability, - ); + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + MEM_REPLACE_OPTION_WITH_NONE, + expr.span, + "replacing an `Option` with `None`", + "consider `Option::take()` instead", + format!("{}.take()", snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)), + applicability, + ); + } + if match_qpath(replacement_qpath, &paths::MEM_UNINITIALIZED) || + match_qpath(replacement_qpath, &paths::MEM_ZEROED) { + span_help_and_lint( + cx, + MEM_REPLACE_WITH_UNINIT, + expr.span, + "replacing with `mem::uninitialized()`", + "consider using the `take_mut` crate instead", + ); + } } } } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 9b88a0d3089b..cda3d2024d2e 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -52,6 +52,8 @@ pub const MEM_FORGET: [&str; 3] = ["core", "mem", "forget"]; pub const MEM_MAYBEUNINIT: [&str; 4] = ["core", "mem", "maybe_uninit", "MaybeUninit"]; pub const MEM_MAYBEUNINIT_UNINIT: [&str; 5] = ["core", "mem", "maybe_uninit", "MaybeUninit", "uninit"]; pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"]; +pub const MEM_UNINITIALIZED: [&str; 3] = ["core", "mem", "uninitialized"]; +pub const MEM_ZEROED: [&str; 3] = ["core", "mem", "zeroed"]; pub const MUTEX: [&str; 4] = ["std", "sync", "mutex", "Mutex"]; pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"]; pub const OPS_MODULE: [&str; 2] = ["core", "ops"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 223e6aa9acd7..847d5e2aa011 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 313] = [ +pub const ALL_LINTS: [Lint; 314] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1043,6 +1043,13 @@ pub const ALL_LINTS: [Lint; 313] = [ deprecation: None, module: "mem_replace", }, + Lint { + name: "mem_replace_with_uninit", + group: "correctness", + desc: "`mem::replace(&mut _, mem::uninitialized())` or `mem::zeroed()`", + deprecation: None, + module: "mem_replace", + }, Lint { name: "min_max", group: "correctness", diff --git a/tests/ui/mem_replace.fixed b/tests/ui/mem_replace.fixed index 4e47ac95d82d..6b638e41583a 100644 --- a/tests/ui/mem_replace.fixed +++ b/tests/ui/mem_replace.fixed @@ -8,14 +8,30 @@ // except according to those terms. // run-rustfix -#![allow(unused_imports)] +#![allow(unused_imports, deprecated, invalid_value)] #![warn(clippy::all, clippy::style, clippy::mem_replace_option_with_none)] use std::mem; +fn might_panic(v: Vec) -> Vec { v } + fn main() { let mut an_option = Some(1); let _ = an_option.take(); let an_option = &mut Some(1); let _ = an_option.take(); + + let mut v = vec![0i32; 4]; + // the following is UB if `might_panic` panics + unsafe { + let taken_v = mem::replace(&mut v, mem::uninitialized()); + let new_v = might_panic(taken_v); + std::mem::forget(mem::replace(&mut v, new_v)); + } + + unsafe { + let taken_v = mem::replace(&mut v, mem::zeroed()); + let new_v = might_panic(taken_v); + std::mem::forget(mem::replace(&mut v, new_v)); + } } diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs index 6824ab18e7fa..3cc9e7fb4e09 100644 --- a/tests/ui/mem_replace.rs +++ b/tests/ui/mem_replace.rs @@ -8,14 +8,30 @@ // except according to those terms. // run-rustfix -#![allow(unused_imports)] +#![allow(unused_imports, deprecated, invalid_value)] #![warn(clippy::all, clippy::style, clippy::mem_replace_option_with_none)] use std::mem; +fn might_panic(v: Vec) -> Vec { v } + fn main() { let mut an_option = Some(1); let _ = mem::replace(&mut an_option, None); let an_option = &mut Some(1); let _ = mem::replace(an_option, None); + + let mut v = vec![0i32; 4]; + // the following is UB if `might_panic` panics + unsafe { + let taken_v = mem::replace(&mut v, mem::uninitialized()); + let new_v = might_panic(taken_v); + std::mem::forget(mem::replace(&mut v, new_v)); + } + + unsafe { + let taken_v = mem::replace(&mut v, mem::zeroed()); + let new_v = might_panic(taken_v); + std::mem::forget(mem::replace(&mut v, new_v)); + } } diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr index 791c4d71dbfc..5652255f357e 100644 --- a/tests/ui/mem_replace.stderr +++ b/tests/ui/mem_replace.stderr @@ -1,5 +1,5 @@ error: replacing an `Option` with `None` - --> $DIR/mem_replace.rs:18:13 + --> $DIR/mem_replace.rs:20:13 | LL | let _ = mem::replace(&mut an_option, None); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()` @@ -7,7 +7,7 @@ LL | let _ = mem::replace(&mut an_option, None); = note: `-D clippy::mem-replace-option-with-none` implied by `-D warnings` error: replacing an `Option` with `None` - --> $DIR/mem_replace.rs:20:13 + --> $DIR/mem_replace.rs:22:13 | LL | let _ = mem::replace(an_option, None); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`