diff --git a/CHANGELOG.md b/CHANGELOG.md index e65e7cc639f..e0f3b82ad25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2124,6 +2124,7 @@ Released 2018-09-13 [`unreadable_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal [`unsafe_derive_deserialize`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_derive_deserialize [`unsafe_removed_from_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_removed_from_name +[`unsafe_sizeof_count_copies`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_sizeof_count_copies [`unsafe_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_vector_initialization [`unseparated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#unseparated_literal_suffix [`unsound_collection_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsound_collection_transmute diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 167e5b6b87f..1bce0130b40 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -329,6 +329,7 @@ mod unnecessary_sort_by; mod unnecessary_wraps; mod unnested_or_patterns; mod unsafe_removed_from_name; +mod unsafe_sizeof_count_copies; mod unused_io_amount; mod unused_self; mod unused_unit; @@ -916,6 +917,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unnecessary_wraps::UNNECESSARY_WRAPS, &unnested_or_patterns::UNNESTED_OR_PATTERNS, &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, + &unsafe_sizeof_count_copies::UNSAFE_SIZEOF_COUNT_COPIES, &unused_io_amount::UNUSED_IO_AMOUNT, &unused_self::UNUSED_SELF, &unused_unit::UNUSED_UNIT, @@ -998,6 +1000,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box matches::Matches::new(msrv)); store.register_early_pass(move || box manual_non_exhaustive::ManualNonExhaustive::new(msrv)); store.register_late_pass(move || box manual_strip::ManualStrip::new(msrv)); + store.register_late_pass(|| box unsafe_sizeof_count_copies::UnsafeSizeofCountCopies); store.register_late_pass(|| box map_clone::MapClone); store.register_late_pass(|| box map_err_ignore::MapErrIgnore); store.register_late_pass(|| box shadow::Shadow); @@ -1605,6 +1608,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS), LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), + LintId::of(&unsafe_sizeof_count_copies::UNSAFE_SIZEOF_COUNT_COPIES), LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT), LintId::of(&unused_unit::UNUSED_UNIT), LintId::of(&unwrap::PANICKING_UNWRAP), @@ -1883,6 +1887,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS), LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS), + LintId::of(&unsafe_sizeof_count_copies::UNSAFE_SIZEOF_COUNT_COPIES), LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT), LintId::of(&unwrap::PANICKING_UNWRAP), LintId::of(&vec_resize_to_zero::VEC_RESIZE_TO_ZERO), diff --git a/clippy_lints/src/unsafe_sizeof_count_copies.rs b/clippy_lints/src/unsafe_sizeof_count_copies.rs new file mode 100644 index 00000000000..2422df8feba --- /dev/null +++ b/clippy_lints/src/unsafe_sizeof_count_copies.rs @@ -0,0 +1,98 @@ +//! Lint on unsafe memory copying that use the `size_of` of the pointee type instead of a pointee +//! count + +use crate::utils::{match_def_path, paths, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir::BinOpKind; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{Ty as TyM, TyS}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Detects expressions where + /// size_of:: is used as the count argument to unsafe + /// memory copying functions like ptr::copy and + /// ptr::copy_nonoverlapping where T is the pointee type + /// of the pointers used + /// + /// **Why is this bad?** These functions expect a count + /// of T and not a number of bytes, which can lead to + /// copying the incorrect amount of bytes, which can + /// result in Undefined Behaviour + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust,no_run + /// # use std::ptr::copy_nonoverlapping; + /// # use std::mem::size_of; + /// + /// const SIZE: usize = 128; + /// let x = [2u8; SIZE]; + /// let mut y = [2u8; SIZE]; + /// unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE) }; + /// ``` + pub UNSAFE_SIZEOF_COUNT_COPIES, + correctness, + "unsafe memory copying using a byte count instead of a count of T" +} + +declare_lint_pass!(UnsafeSizeofCountCopies => [UNSAFE_SIZEOF_COUNT_COPIES]); + +fn get_size_of_ty(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { + match &expr.kind { + ExprKind::Call(ref count_func, _func_args) => { + if_chain! { + if let ExprKind::Path(ref count_func_qpath) = count_func.kind; + if let Some(def_id) = cx.qpath_res(count_func_qpath, count_func.hir_id).opt_def_id(); + if match_def_path(cx, def_id, &paths::MEM_SIZE_OF) + || match_def_path(cx, def_id, &paths::MEM_SIZE_OF_VAL); + then { + cx.typeck_results().node_substs(count_func.hir_id).types().next() + } else { + None + } + } + }, + ExprKind::Binary(op, left, right) if BinOpKind::Mul == op.node || BinOpKind::Div == op.node => { + get_size_of_ty(cx, &*left).or_else(|| get_size_of_ty(cx, &*right)) + }, + _ => None, + } +} + +impl<'tcx> LateLintPass<'tcx> for UnsafeSizeofCountCopies { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + // Find calls to ptr::copy and copy_nonoverlapping + if let ExprKind::Call(ref func, ref func_args) = expr.kind; + if let ExprKind::Path(ref func_qpath) = func.kind; + if let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id(); + if match_def_path(cx, def_id, &paths::COPY_NONOVERLAPPING) + || match_def_path(cx, def_id, &paths::COPY); + + // Get the pointee type + let _substs = cx.typeck_results().node_substs(func.hir_id); + if let Some(pointee_ty) = cx.typeck_results().node_substs(func.hir_id).types().next(); + + // Find a size_of call in the count parameter expression and + // check that it's the same type + if let [_src, _dest, count] = &**func_args; + if let Some(ty_used_for_size_of) = get_size_of_ty(cx, count); + if TyS::same_type(pointee_ty, ty_used_for_size_of); + then { + span_lint_and_help( + cx, + UNSAFE_SIZEOF_COUNT_COPIES, + expr.span, + "unsafe memory copying using a byte count (Multiplied by size_of::) \ + instead of a count of T", + None, + "use a count of elements instead of a count of bytes for the count parameter, \ + it already gets multiplied by the size of the pointed to type" + ); + } + }; + } +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 16e6a016c9e..fe763d4bfbb 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -20,6 +20,8 @@ pub const CLONE_TRAIT: [&str; 3] = ["core", "clone", "Clone"]; pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"]; pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"]; +pub const COPY: [&str; 3] = ["core", "intrinsics", "copy_nonoverlapping"]; +pub const COPY_NONOVERLAPPING: [&str; 3] = ["core", "intrinsics", "copy"]; pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"]; pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"]; pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"]; @@ -73,6 +75,8 @@ pub const MEM_MANUALLY_DROP: [&str; 4] = ["core", "mem", "manually_drop", "Manua 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_SIZE_OF: [&str; 3] = ["core", "mem", "size_of"]; +pub const MEM_SIZE_OF_VAL: [&str; 3] = ["core", "mem", "size_of_val"]; pub const MUTEX_GUARD: [&str; 4] = ["std", "sync", "mutex", "MutexGuard"]; pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"]; pub const OPS_MODULE: [&str; 2] = ["core", "ops"]; diff --git a/tests/ui/unsafe_sizeof_count_copies.rs b/tests/ui/unsafe_sizeof_count_copies.rs new file mode 100644 index 00000000000..0077ed07fce --- /dev/null +++ b/tests/ui/unsafe_sizeof_count_copies.rs @@ -0,0 +1,54 @@ +#![warn(clippy::unsafe_sizeof_count_copies)] + +use std::mem::{size_of, size_of_val}; +use std::ptr::{copy, copy_nonoverlapping}; + +fn main() { + const SIZE: usize = 128; + const HALF_SIZE: usize = SIZE / 2; + const DOUBLE_SIZE: usize = SIZE * 2; + let mut x = [2u8; SIZE]; + let mut y = [2u8; SIZE]; + + // Count is size_of (Should trigger the lint) + unsafe { copy_nonoverlapping::(x.as_ptr(), y.as_mut_ptr(), size_of::()) }; + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) }; + + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::()) }; + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) }; + + // Count expression involving multiplication of size_of (Should trigger the lint) + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE) }; + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * SIZE) }; + + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE) }; + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * SIZE) }; + + // Count expression involving nested multiplications of size_of (Should trigger the lint) + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * HALF_SIZE * 2) }; + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), HALF_SIZE * size_of_val(&x[0]) * 2) }; + + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE * HALF_SIZE) }; + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * HALF_SIZE * 2) }; + + // Count expression involving divisions of size_of (Should trigger the lint) + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * DOUBLE_SIZE / 2) }; + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE / 2 * size_of_val(&x[0])) }; + + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE * size_of::() / 2) }; + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * DOUBLE_SIZE / 2) }; + + // No size_of calls (Should not trigger the lint) + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), SIZE) }; + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), SIZE) }; + + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), SIZE) }; + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), SIZE) }; + + // Different types for pointee and size_of (Should not trigger the lint) + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() / 2 * SIZE) }; + unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&0u16) / 2 * SIZE) }; + + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::() / 2 * SIZE) }; + unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&0u16) / 2 * SIZE) }; +} diff --git a/tests/ui/unsafe_sizeof_count_copies.stderr b/tests/ui/unsafe_sizeof_count_copies.stderr new file mode 100644 index 00000000000..6804df8cdfc --- /dev/null +++ b/tests/ui/unsafe_sizeof_count_copies.stderr @@ -0,0 +1,131 @@ +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:14:14 + | +LL | unsafe { copy_nonoverlapping::(x.as_ptr(), y.as_mut_ptr(), size_of::()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unsafe-sizeof-count-copies` implied by `-D warnings` + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:15:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:17:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:18:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:21:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:22:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * SIZE) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:24:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:25:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * SIZE) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:28:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * HALF_SIZE * 2) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:29:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), HALF_SIZE * size_of_val(&x[0]) * 2) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:31:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::() * SIZE * HALF_SIZE) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:32:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * HALF_SIZE * 2) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:35:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::() * DOUBLE_SIZE / 2) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:36:14 + | +LL | unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE / 2 * size_of_val(&x[0])) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:38:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE * size_of::() / 2) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: unsafe memory copying using a byte count (Multiplied by size_of::) instead of a count of T + --> $DIR/unsafe_sizeof_count_copies.rs:39:14 + | +LL | unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * DOUBLE_SIZE / 2) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use a count of elements instead of a count of bytes for the count parameter, it already gets multiplied by the size of the pointed to type + +error: aborting due to 16 previous errors +