Auto merge of #7023 - boxdot:invalid-null-usage-v2, r=camsteffen

Invalid null usage v2

This is continuation of #6192 after inactivity.

I plan to move paths into the compiler as diagnostic items after this is merged.

fixes #1703
changelog: none

Author: bors

CHANGELOG.md

@@ -2267,6 +2267,7 @@ Released 2018-09-13
 [`into_iter_on_array`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array
 [`into_iter_on_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref
 [`invalid_atomic_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering
+[`invalid_null_ptr_usage`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_null_ptr_usage
 [`invalid_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_ref
 [`invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_regex
 [`invalid_upcast_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons

clippy_lints/src/lib.rs

@@ -903,6 +903,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         pattern_type_mismatch::PATTERN_TYPE_MISMATCH,
         precedence::PRECEDENCE,
         ptr::CMP_NULL,
+        ptr::INVALID_NULL_PTR_USAGE,
         ptr::MUT_FROM_REF,
         ptr::PTR_ARG,
         ptr_eq::PTR_EQ,
@@ -1672,6 +1673,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL),
         LintId::of(precedence::PRECEDENCE),
         LintId::of(ptr::CMP_NULL),
+        LintId::of(ptr::INVALID_NULL_PTR_USAGE),
         LintId::of(ptr::MUT_FROM_REF),
         LintId::of(ptr::PTR_ARG),
         LintId::of(ptr_eq::PTR_EQ),
@@ -2011,6 +2013,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS),
         LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS),
         LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP),
+        LintId::of(ptr::INVALID_NULL_PTR_USAGE),
         LintId::of(ptr::MUT_FROM_REF),
         LintId::of(ranges::REVERSED_EMPTY_RANGES),
         LintId::of(regex::INVALID_REGEX),

clippy_lints/src/ptr.rs

@@ -4,7 +4,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_the
 use clippy_utils::ptr::get_spans;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{is_type_diagnostic_item, match_type, walk_ptrs_hir_ty};
-use clippy_utils::{is_allowed, match_qpath, paths};
+use clippy_utils::{is_allowed, match_def_path, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{
@@ -15,6 +15,7 @@ use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
+use rustc_span::symbol::Symbol;
 use rustc_span::{sym, MultiSpan};
 use std::borrow::Cow;
 
@@ -94,7 +95,7 @@ declare_clippy_lint! {
     /// ```
     pub CMP_NULL,
     style,
-    "comparing a pointer to a null pointer, suggesting to use `.is_null()` instead."
+    "comparing a pointer to a null pointer, suggesting to use `.is_null()` instead"
 }
 
 declare_clippy_lint! {
@@ -119,7 +120,28 @@ declare_clippy_lint! {
     "fns that create mutable refs from immutable ref args"
 }
 
-declare_lint_pass!(Ptr => [PTR_ARG, CMP_NULL, MUT_FROM_REF]);
+declare_clippy_lint! {
+    /// **What it does:** This lint checks for invalid usages of `ptr::null`.
+    ///
+    /// **Why is this bad?** This causes undefined behavior.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```ignore
+    /// // Bad. Undefined behavior
+    /// unsafe { std::slice::from_raw_parts(ptr::null(), 0); }
+    /// ```
+    ///
+    /// // Good
+    /// unsafe { std::slice::from_raw_parts(NonNull::dangling().as_ptr(), 0); }
+    /// ```
+    pub INVALID_NULL_PTR_USAGE,
+    correctness,
+    "invalid usage of a null pointer, suggesting `NonNull::dangling()` instead"
+}
+
+declare_lint_pass!(Ptr => [PTR_ARG, CMP_NULL, MUT_FROM_REF, INVALID_NULL_PTR_USAGE]);
 
 impl<'tcx> LateLintPass<'tcx> for Ptr {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
@@ -153,14 +175,63 @@ impl<'tcx> LateLintPass<'tcx> for Ptr {
 
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if let ExprKind::Binary(ref op, l, r) = expr.kind {
-            if (op.node == BinOpKind::Eq || op.node == BinOpKind::Ne) && (is_null_path(l) || is_null_path(r)) {
+            if (op.node == BinOpKind::Eq || op.node == BinOpKind::Ne) && (is_null_path(cx, l) || is_null_path(cx, r)) {
                 span_lint(
                     cx,
                     CMP_NULL,
                     expr.span,
                     "comparing with null is better expressed by the `.is_null()` method",
                 );
             }
+        } else {
+            check_invalid_ptr_usage(cx, expr);
+        }
+    }
+}
+
+fn check_invalid_ptr_usage<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+    // (fn_path, arg_indices) - `arg_indices` are the `arg` positions where null would cause U.B.
+    const INVALID_NULL_PTR_USAGE_TABLE: [(&[&str], &[usize]); 16] = [
+        (&paths::SLICE_FROM_RAW_PARTS, &[0]),
+        (&paths::SLICE_FROM_RAW_PARTS_MUT, &[0]),
+        (&paths::PTR_COPY, &[0, 1]),
+        (&paths::PTR_COPY_NONOVERLAPPING, &[0, 1]),
+        (&paths::PTR_READ, &[0]),
+        (&paths::PTR_READ_UNALIGNED, &[0]),
+        (&paths::PTR_READ_VOLATILE, &[0]),
+        (&paths::PTR_REPLACE, &[0]),
+        (&paths::PTR_SLICE_FROM_RAW_PARTS, &[0]),
+        (&paths::PTR_SLICE_FROM_RAW_PARTS_MUT, &[0]),
+        (&paths::PTR_SWAP, &[0, 1]),
+        (&paths::PTR_SWAP_NONOVERLAPPING, &[0, 1]),
+        (&paths::PTR_WRITE, &[0]),
+        (&paths::PTR_WRITE_UNALIGNED, &[0]),
+        (&paths::PTR_WRITE_VOLATILE, &[0]),
+        (&paths::PTR_WRITE_BYTES, &[0]),
+    ];
+
+    if_chain! {
+        if let ExprKind::Call(ref fun, ref args) = expr.kind;
+        if let ExprKind::Path(ref qpath) = fun.kind;
+        if let Some(fun_def_id) = cx.qpath_res(qpath, fun.hir_id).opt_def_id();
+        let fun_def_path = cx.get_def_path(fun_def_id).into_iter().map(Symbol::to_ident_string).collect::<Vec<_>>();
+        if let Some(&(_, arg_indices)) = INVALID_NULL_PTR_USAGE_TABLE
+            .iter()
+            .find(|&&(fn_path, _)| fn_path == fun_def_path);
+        then {
+            for &arg_idx in arg_indices {
+                if let Some(arg) = args.get(arg_idx).filter(|arg| is_null_path(cx, arg)) {
+                    span_lint_and_sugg(
+                        cx,
+                        INVALID_NULL_PTR_USAGE,
+                        arg.span,
+                        "pointer must be non-null",
+                        "change this to",
+                        "core::ptr::NonNull::dangling().as_ptr()".to_string(),
+                        Applicability::MachineApplicable,
+                    );
+                }
+            }
         }
     }
 }
@@ -345,13 +416,15 @@ fn get_rptr_lm<'tcx>(ty: &'tcx Ty<'tcx>) -> Option<(&'tcx Lifetime, Mutability,
     }
 }
 
-fn is_null_path(expr: &Expr<'_>) -> bool {
-    if let ExprKind::Call(pathexp, args) = expr.kind {
-        if args.is_empty() {
-            if let ExprKind::Path(ref path) = pathexp.kind {
-                return match_qpath(path, &paths::PTR_NULL) || match_qpath(path, &paths::PTR_NULL_MUT);
-            }
+fn is_null_path(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if_chain! {
+        if let ExprKind::Call(path, []) = expr.kind;
+        if let ExprKind::Path(ref qpath) = path.kind;
+        if let Some(fn_def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id();
+        then {
+            match_def_path(cx, fn_def_id, &paths::PTR_NULL) || match_def_path(cx, fn_def_id, &paths::PTR_NULL_MUT)
+        } else {
+            false
         }
     }
-    false
 }

clippy_lints/src/size_of_in_element_count.rs

@@ -65,8 +65,8 @@ fn get_size_of_ty(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, inverted: bool)
 
 fn get_pointee_ty_and_count_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<(Ty<'tcx>, &'tcx Expr<'tcx>)> {
     const FUNCTIONS: [&[&str]; 8] = [
-        &paths::COPY_NONOVERLAPPING,
-        &paths::COPY,
+        &paths::PTR_COPY_NONOVERLAPPING,
+        &paths::PTR_COPY,
         &paths::WRITE_BYTES,
         &paths::PTR_SWAP_NONOVERLAPPING,
         &paths::PTR_SLICE_FROM_RAW_PARTS,

clippy_utils/src/paths.rs

@@ -18,8 +18,6 @@ pub const BTREESET: [&str; 5] = ["alloc", "collections", "btree", "set", "BTreeS
 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; 4] = ["core", "intrinsics", "", "copy_nonoverlapping"];
-pub const COPY_NONOVERLAPPING: [&str; 4] = ["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"];
@@ -100,12 +98,23 @@ pub const PERMISSIONS_FROM_MODE: [&str; 7] = ["std", "sys", "unix", "ext", "fs",
 pub const POLL: [&str; 4] = ["core", "task", "poll", "Poll"];
 pub const POLL_PENDING: [&str; 5] = ["core", "task", "poll", "Poll", "Pending"];
 pub const POLL_READY: [&str; 5] = ["core", "task", "poll", "Poll", "Ready"];
+pub const PTR_COPY: [&str; 4] = ["core", "intrinsics", "", "copy"];
+pub const PTR_COPY_NONOVERLAPPING: [&str; 4] = ["core", "intrinsics", "", "copy_nonoverlapping"];
 pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
 pub const PTR_NULL: [&str; 3] = ["core", "ptr", "null"];
 pub const PTR_NULL_MUT: [&str; 3] = ["core", "ptr", "null_mut"];
 pub const PTR_SLICE_FROM_RAW_PARTS: [&str; 3] = ["core", "ptr", "slice_from_raw_parts"];
 pub const PTR_SLICE_FROM_RAW_PARTS_MUT: [&str; 3] = ["core", "ptr", "slice_from_raw_parts_mut"];
 pub const PTR_SWAP_NONOVERLAPPING: [&str; 3] = ["core", "ptr", "swap_nonoverlapping"];
+pub const PTR_READ: [&str; 3] = ["core", "ptr", "read"];
+pub const PTR_READ_UNALIGNED: [&str; 3] = ["core", "ptr", "read_unaligned"];
+pub const PTR_READ_VOLATILE: [&str; 3] = ["core", "ptr", "read_volatile"];
+pub const PTR_REPLACE: [&str; 3] = ["core", "ptr", "replace"];
+pub const PTR_SWAP: [&str; 3] = ["core", "ptr", "swap"];
+pub const PTR_WRITE: [&str; 3] = ["core", "ptr", "write"];
+pub const PTR_WRITE_BYTES: [&str; 3] = ["core", "intrinsics", "write_bytes"];
+pub const PTR_WRITE_UNALIGNED: [&str; 3] = ["core", "ptr", "write_unaligned"];
+pub const PTR_WRITE_VOLATILE: [&str; 3] = ["core", "ptr", "write_volatile"];
 pub const PUSH_STR: [&str; 4] = ["alloc", "string", "String", "push_str"];
 pub const RANGE_ARGUMENT_TRAIT: [&str; 3] = ["core", "ops", "RangeBounds"];
 pub const RC_PTR_EQ: [&str; 4] = ["alloc", "rc", "Rc", "ptr_eq"];

tests/ui/invalid_null_ptr_usage.fixed

@@ -0,0 +1,49 @@
+// run-rustfix
+
+fn main() {
+    unsafe {
+        let _slice: &[usize] = std::slice::from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);
+        let _slice: &[usize] = std::slice::from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        let _slice: &[usize] = std::slice::from_raw_parts_mut(core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        std::ptr::copy::<usize>(core::ptr::NonNull::dangling().as_ptr(), std::ptr::NonNull::dangling().as_ptr(), 0);
+        std::ptr::copy::<usize>(std::ptr::NonNull::dangling().as_ptr(), core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        std::ptr::copy_nonoverlapping::<usize>(core::ptr::NonNull::dangling().as_ptr(), std::ptr::NonNull::dangling().as_ptr(), 0);
+        std::ptr::copy_nonoverlapping::<usize>(std::ptr::NonNull::dangling().as_ptr(), core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        struct A; // zero sized struct
+        assert_eq!(std::mem::size_of::<A>(), 0);
+
+        let _a: A = std::ptr::read(core::ptr::NonNull::dangling().as_ptr());
+        let _a: A = std::ptr::read(core::ptr::NonNull::dangling().as_ptr());
+
+        let _a: A = std::ptr::read_unaligned(core::ptr::NonNull::dangling().as_ptr());
+        let _a: A = std::ptr::read_unaligned(core::ptr::NonNull::dangling().as_ptr());
+
+        let _a: A = std::ptr::read_volatile(core::ptr::NonNull::dangling().as_ptr());
+        let _a: A = std::ptr::read_volatile(core::ptr::NonNull::dangling().as_ptr());
+
+        let _a: A = std::ptr::replace(core::ptr::NonNull::dangling().as_ptr(), A);
+
+        let _slice: *const [usize] = std::ptr::slice_from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);
+        let _slice: *const [usize] = std::ptr::slice_from_raw_parts(core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        let _slice: *const [usize] = std::ptr::slice_from_raw_parts_mut(core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        std::ptr::swap::<A>(core::ptr::NonNull::dangling().as_ptr(), &mut A);
+        std::ptr::swap::<A>(&mut A, core::ptr::NonNull::dangling().as_ptr());
+
+        std::ptr::swap_nonoverlapping::<A>(core::ptr::NonNull::dangling().as_ptr(), &mut A, 0);
+        std::ptr::swap_nonoverlapping::<A>(&mut A, core::ptr::NonNull::dangling().as_ptr(), 0);
+
+        std::ptr::write(core::ptr::NonNull::dangling().as_ptr(), A);
+
+        std::ptr::write_unaligned(core::ptr::NonNull::dangling().as_ptr(), A);
+
+        std::ptr::write_volatile(core::ptr::NonNull::dangling().as_ptr(), A);
+
+        std::ptr::write_bytes::<usize>(core::ptr::NonNull::dangling().as_ptr(), 42, 0);
+    }
+}

tests/ui/invalid_null_ptr_usage.rs

@@ -0,0 +1,49 @@
+// run-rustfix
+
+fn main() {
+    unsafe {
+        let _slice: &[usize] = std::slice::from_raw_parts(std::ptr::null(), 0);
+        let _slice: &[usize] = std::slice::from_raw_parts(std::ptr::null_mut(), 0);
+
+        let _slice: &[usize] = std::slice::from_raw_parts_mut(std::ptr::null_mut(), 0);
+
+        std::ptr::copy::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
+        std::ptr::copy::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);
+
+        std::ptr::copy_nonoverlapping::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
+        std::ptr::copy_nonoverlapping::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);
+
+        struct A; // zero sized struct
+        assert_eq!(std::mem::size_of::<A>(), 0);
+
+        let _a: A = std::ptr::read(std::ptr::null());
+        let _a: A = std::ptr::read(std::ptr::null_mut());
+
+        let _a: A = std::ptr::read_unaligned(std::ptr::null());
+        let _a: A = std::ptr::read_unaligned(std::ptr::null_mut());
+
+        let _a: A = std::ptr::read_volatile(std::ptr::null());
+        let _a: A = std::ptr::read_volatile(std::ptr::null_mut());
+
+        let _a: A = std::ptr::replace(std::ptr::null_mut(), A);
+
+        let _slice: *const [usize] = std::ptr::slice_from_raw_parts(std::ptr::null(), 0);
+        let _slice: *const [usize] = std::ptr::slice_from_raw_parts(std::ptr::null_mut(), 0);
+
+        let _slice: *const [usize] = std::ptr::slice_from_raw_parts_mut(std::ptr::null_mut(), 0);
+
+        std::ptr::swap::<A>(std::ptr::null_mut(), &mut A);
+        std::ptr::swap::<A>(&mut A, std::ptr::null_mut());
+
+        std::ptr::swap_nonoverlapping::<A>(std::ptr::null_mut(), &mut A, 0);
+        std::ptr::swap_nonoverlapping::<A>(&mut A, std::ptr::null_mut(), 0);
+
+        std::ptr::write(std::ptr::null_mut(), A);
+
+        std::ptr::write_unaligned(std::ptr::null_mut(), A);
+
+        std::ptr::write_volatile(std::ptr::null_mut(), A);
+
+        std::ptr::write_bytes::<usize>(std::ptr::null_mut(), 42, 0);
+    }
+}