Auto merge of #10099 - Niki4tap:null_fn_lints, r=llogiq

bors · bors · commit b3145fea6a5c · 2022-12-19T12:44:23.000Z
Null fn lints Adds lints to check for code, that assumes nullable `fn()`. ### Lint examples: `transmute_null_to_fn`: ```rust error: transmuting a known null pointer into a function pointer --> $DIR/transmute_null_to_fn.rs:9:23 | LL | let _: fn() = std::mem::transmute(std::ptr::null::<()>()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this transmute results in undefined behavior | = help: try wrapping your function pointer type in `Option<T>` instead, and using `None` as a null pointer value ``` `fn_null_check`: ```rust error: function pointer assumed to be nullable, even though it isn't --> $DIR/fn_null_check.rs:13:8 | LL | if (fn_ptr as *mut ()).is_null() {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value ``` Closes #1644 --- changelog: Improvement: [`transmuting_null`]: Now detects `const` pointers to all types [#10099](#10099) changelog: New lint: [`transmute_null_to_fn`] [#10099](#10099) changelog: New lint: [`fn_null_check`] [#10099](#10099)
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4203,6 +4203,7 @@ Released 2018-09-13
 [`float_cmp_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp_const
 [`float_equality_without_abs`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_equality_without_abs
 [`fn_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_address_comparisons
+[`fn_null_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_null_check
 [`fn_params_excessive_bools`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_params_excessive_bools
 [`fn_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast
 [`fn_to_numeric_cast_any`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_any
@@ -4590,6 +4591,7 @@ Released 2018-09-13
 [`transmute_int_to_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_int_to_bool
 [`transmute_int_to_char`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_int_to_char
 [`transmute_int_to_float`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_int_to_float
+[`transmute_null_to_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_null_to_fn
 [`transmute_num_to_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_num_to_bytes
 [`transmute_ptr_to_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_ptr_to_ptr
 [`transmute_ptr_to_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_ptr_to_ref
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -161,6 +161,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::float_literal::LOSSY_FLOAT_LITERAL_INFO,
     crate::floating_point_arithmetic::IMPRECISE_FLOPS_INFO,
     crate::floating_point_arithmetic::SUBOPTIMAL_FLOPS_INFO,
+    crate::fn_null_check::FN_NULL_CHECK_INFO,
     crate::format::USELESS_FORMAT_INFO,
     crate::format_args::FORMAT_IN_FORMAT_ARGS_INFO,
     crate::format_args::TO_STRING_IN_FORMAT_ARGS_INFO,
@@ -568,6 +569,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::transmute::TRANSMUTE_INT_TO_BOOL_INFO,
     crate::transmute::TRANSMUTE_INT_TO_CHAR_INFO,
     crate::transmute::TRANSMUTE_INT_TO_FLOAT_INFO,
+    crate::transmute::TRANSMUTE_NULL_TO_FN_INFO,
     crate::transmute::TRANSMUTE_NUM_TO_BYTES_INFO,
     crate::transmute::TRANSMUTE_PTR_TO_PTR_INFO,
     crate::transmute::TRANSMUTE_PTR_TO_REF_INFO,
diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs
@@ -0,0 +1,106 @@
+use clippy_utils::consts::{constant, Constant};
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::{is_integer_literal, is_path_diagnostic_item};
+use rustc_hir::{BinOpKind, Expr, ExprKind, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for comparing a function pointer to null.
+    ///
+    /// ### Why is this bad?
+    /// Function pointers are assumed to not be null.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// let fn_ptr: fn() = /* somehow obtained nullable function pointer */
+    ///
+    /// if (fn_ptr as *const ()).is_null() { ... }
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// let fn_ptr: Option<fn()> = /* somehow obtained nullable function pointer */
+    ///
+    /// if fn_ptr.is_none() { ... }
+    /// ```
+    #[clippy::version = "1.67.0"]
+    pub FN_NULL_CHECK,
+    correctness,
+    "`fn()` type assumed to be nullable"
+}
+declare_lint_pass!(FnNullCheck => [FN_NULL_CHECK]);
+
+fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) {
+    span_lint_and_help(
+        cx,
+        FN_NULL_CHECK,
+        expr.span,
+        "function pointer assumed to be nullable, even though it isn't",
+        None,
+        "try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value",
+    );
+}
+
+fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let ExprKind::Cast(cast_expr, cast_ty) = expr.kind
+        && let TyKind::Ptr(_) = cast_ty.kind
+    {
+        cx.typeck_results().expr_ty_adjusted(cast_expr).is_fn()
+    } else {
+        false
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for FnNullCheck {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        match expr.kind {
+            // Catching:
+            // (fn_ptr as *<const/mut> <ty>).is_null()
+            ExprKind::MethodCall(method_name, receiver, _, _)
+                if method_name.ident.as_str() == "is_null" && is_fn_ptr_cast(cx, receiver) =>
+            {
+                lint_expr(cx, expr);
+            },
+
+            ExprKind::Binary(op, left, right) if matches!(op.node, BinOpKind::Eq) => {
+                let to_check: &Expr<'_>;
+                if is_fn_ptr_cast(cx, left) {
+                    to_check = right;
+                } else if is_fn_ptr_cast(cx, right) {
+                    to_check = left;
+                } else {
+                    return;
+                }
+
+                match to_check.kind {
+                    // Catching:
+                    // (fn_ptr as *<const/mut> <ty>) == (0 as <ty>)
+                    ExprKind::Cast(cast_expr, _) if is_integer_literal(cast_expr, 0) => {
+                        lint_expr(cx, expr);
+                    },
+
+                    // Catching:
+                    // (fn_ptr as *<const/mut> <ty>) == std::ptr::null()
+                    ExprKind::Call(func, []) if is_path_diagnostic_item(cx, func, sym::ptr_null) => {
+                        lint_expr(cx, expr);
+                    },
+
+                    // Catching:
+                    // (fn_ptr as *<const/mut> <ty>) == <const that evaluates to null_ptr>
+                    _ if matches!(
+                        constant(cx, cx.typeck_results(), to_check),
+                        Some((Constant::RawPtr(0), _))
+                    ) =>
+                    {
+                        lint_expr(cx, expr);
+                    },
+
+                    _ => {},
+                }
+            },
+            _ => {},
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -125,6 +125,7 @@ mod explicit_write;
 mod fallible_impl_from;
 mod float_literal;
 mod floating_point_arithmetic;
+mod fn_null_check;
 mod format;
 mod format_args;
 mod format_impl;
@@ -902,6 +903,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(suspicious_xor_used_as_pow::ConfusingXorAndPow));
     store.register_late_pass(move |_| Box::new(manual_is_ascii_check::ManualIsAsciiCheck::new(msrv())));
     store.register_late_pass(|_| Box::new(semicolon_block::SemicolonBlock));
+    store.register_late_pass(|_| Box::new(fn_null_check::FnNullCheck));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/transmute/mod.rs b/clippy_lints/src/transmute/mod.rs
@@ -3,6 +3,7 @@ mod transmute_float_to_int;
 mod transmute_int_to_bool;
 mod transmute_int_to_char;
 mod transmute_int_to_float;
+mod transmute_null_to_fn;
 mod transmute_num_to_bytes;
 mod transmute_ptr_to_ptr;
 mod transmute_ptr_to_ref;
@@ -409,6 +410,34 @@ declare_clippy_lint! {
     "transmutes from a null pointer to a reference, which is undefined behavior"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for null function pointer creation through transmute.
+    ///
+    /// ### Why is this bad?
+    /// Creating a null function pointer is undefined behavior.
+    ///
+    /// More info: https://doc.rust-lang.org/nomicon/ffi.html#the-nullable-pointer-optimization
+    ///
+    /// ### Known problems
+    /// Not all cases can be detected at the moment of this writing.
+    /// For example, variables which hold a null pointer and are then fed to a `transmute`
+    /// call, aren't detectable yet.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let null_fn: fn() = unsafe { std::mem::transmute( std::ptr::null::<()>() ) };
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let null_fn: Option<fn()> = None;
+    /// ```
+    #[clippy::version = "1.67.0"]
+    pub TRANSMUTE_NULL_TO_FN,
+    correctness,
+    "transmute results in a null function pointer, which is undefined behavior"
+}
+
 pub struct Transmute {
     msrv: Msrv,
 }
@@ -428,6 +457,7 @@ impl_lint_pass!(Transmute => [
     TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS,
     TRANSMUTE_UNDEFINED_REPR,
     TRANSMUTING_NULL,
+    TRANSMUTE_NULL_TO_FN,
 ]);
 impl Transmute {
     #[must_use]
@@ -461,6 +491,7 @@ impl<'tcx> LateLintPass<'tcx> for Transmute {
                 let linted = wrong_transmute::check(cx, e, from_ty, to_ty)
                     | crosspointer_transmute::check(cx, e, from_ty, to_ty)
                     | transmuting_null::check(cx, e, arg, to_ty)
+                    | transmute_null_to_fn::check(cx, e, arg, to_ty)
                     | transmute_ptr_to_ref::check(cx, e, from_ty, to_ty, arg, path, &self.msrv)
                     | transmute_int_to_char::check(cx, e, from_ty, to_ty, arg, const_context)
                     | transmute_ref_to_ref::check(cx, e, from_ty, to_ty, arg, const_context)
diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs
@@ -0,0 +1,64 @@
+use clippy_utils::consts::{constant, Constant};
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::{is_integer_literal, is_path_diagnostic_item};
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty::Ty;
+use rustc_span::symbol::sym;
+
+use super::TRANSMUTE_NULL_TO_FN;
+
+fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) {
+    span_lint_and_then(
+        cx,
+        TRANSMUTE_NULL_TO_FN,
+        expr.span,
+        "transmuting a known null pointer into a function pointer",
+        |diag| {
+            diag.span_label(expr.span, "this transmute results in undefined behavior");
+            diag.help(
+               "try wrapping your function pointer type in `Option<T>` instead, and using `None` as a null pointer value"
+            );
+        },
+    );
+}
+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arg: &'tcx Expr<'_>, to_ty: Ty<'tcx>) -> bool {
+    if !to_ty.is_fn() {
+        return false;
+    }
+
+    match arg.kind {
+        // Catching:
+        // transmute over constants that resolve to `null`.
+        ExprKind::Path(ref _qpath)
+            if matches!(constant(cx, cx.typeck_results(), arg), Some((Constant::RawPtr(0), _))) =>
+        {
+            lint_expr(cx, expr);
+            true
+        },
+
+        // Catching:
+        // `std::mem::transmute(0 as *const i32)`
+        ExprKind::Cast(inner_expr, _cast_ty) if is_integer_literal(inner_expr, 0) => {
+            lint_expr(cx, expr);
+            true
+        },
+
+        // Catching:
+        // `std::mem::transmute(std::ptr::null::<i32>())`
+        ExprKind::Call(func1, []) if is_path_diagnostic_item(cx, func1, sym::ptr_null) => {
+            lint_expr(cx, expr);
+            true
+        },
+
+        _ => {
+            // FIXME:
+            // Also catch transmutations of variables which are known nulls.
+            // To do this, MIR const propagation seems to be the better tool.
+            // Whenever MIR const prop routines are more developed, this will
+            // become available. As of this writing (25/03/19) it is not yet.
+            false
+        },
+    }
+}
diff --git a/clippy_lints/src/transmute/transmuting_null.rs b/clippy_lints/src/transmute/transmuting_null.rs
@@ -18,8 +18,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arg: &'t
     // Catching transmute over constants that resolve to `null`.
     let mut const_eval_context = constant_context(cx, cx.typeck_results());
     if let ExprKind::Path(ref _qpath) = arg.kind &&
-        let Some(Constant::RawPtr(x)) = const_eval_context.expr(arg) &&
-        x == 0
+        let Some(Constant::RawPtr(0)) = const_eval_context.expr(arg)
     {
         span_lint(cx, TRANSMUTING_NULL, expr.span, LINT_MSG);
         return true;
diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs
@@ -620,12 +620,7 @@ pub fn miri_to_const<'tcx>(tcx: TyCtxt<'tcx>, result: mir::ConstantKind<'tcx>) -
                 ty::Float(FloatTy::F64) => Some(Constant::F64(f64::from_bits(
                     int.try_into().expect("invalid f64 bit representation"),
                 ))),
-                ty::RawPtr(type_and_mut) => {
-                    if let ty::Uint(_) = type_and_mut.ty.kind() {
-                        return Some(Constant::RawPtr(int.assert_bits(int.size())));
-                    }
-                    None
-                },
+                ty::RawPtr(_) => Some(Constant::RawPtr(int.assert_bits(int.size()))),
                 // FIXME: implement other conversions.
                 _ => None,
             }
diff --git a/tests/ui/fn_null_check.rs b/tests/ui/fn_null_check.rs
@@ -0,0 +1,21 @@
+#![allow(unused)]
+#![warn(clippy::fn_null_check)]
+#![allow(clippy::cmp_null)]
+#![allow(clippy::ptr_eq)]
+#![allow(clippy::zero_ptr)]
+
+pub const ZPTR: *const () = 0 as *const _;
+pub const NOT_ZPTR: *const () = 1 as *const _;
+
+fn main() {
+    let fn_ptr = main;
+
+    if (fn_ptr as *mut ()).is_null() {}
+    if (fn_ptr as *const u8).is_null() {}
+    if (fn_ptr as *const ()) == std::ptr::null() {}
+    if (fn_ptr as *const ()) == (0 as *const ()) {}
+    if (fn_ptr as *const ()) == ZPTR {}
+
+    // no lint
+    if (fn_ptr as *const ()) == NOT_ZPTR {}
+}
diff --git a/tests/ui/fn_null_check.stderr b/tests/ui/fn_null_check.stderr
@@ -0,0 +1,43 @@
+error: function pointer assumed to be nullable, even though it isn't
+  --> $DIR/fn_null_check.rs:13:8
+   |
+LL |     if (fn_ptr as *mut ()).is_null() {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value
+   = note: `-D clippy::fn-null-check` implied by `-D warnings`
+
+error: function pointer assumed to be nullable, even though it isn't
+  --> $DIR/fn_null_check.rs:14:8
+   |
+LL |     if (fn_ptr as *const u8).is_null() {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value
+
+error: function pointer assumed to be nullable, even though it isn't
+  --> $DIR/fn_null_check.rs:15:8
+   |
+LL |     if (fn_ptr as *const ()) == std::ptr::null() {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value
+
+error: function pointer assumed to be nullable, even though it isn't
+  --> $DIR/fn_null_check.rs:16:8
+   |
+LL |     if (fn_ptr as *const ()) == (0 as *const ()) {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value
+
+error: function pointer assumed to be nullable, even though it isn't
+  --> $DIR/fn_null_check.rs:17:8
+   |
+LL |     if (fn_ptr as *const ()) == ZPTR {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: try wrapping your function pointer type in `Option<T>` instead, and using `is_none` to check for null pointer value
+
+error: aborting due to 5 previous errors
+
diff --git a/tests/ui/transmute_null_to_fn.rs b/tests/ui/transmute_null_to_fn.rs
@@ -0,0 +1,28 @@
+#![allow(dead_code)]
+#![warn(clippy::transmute_null_to_fn)]
+#![allow(clippy::zero_ptr)]
+
+// Easy to lint because these only span one line.
+fn one_liners() {
+    unsafe {
+        let _: fn() = std::mem::transmute(0 as *const ());
+        let _: fn() = std::mem::transmute(std::ptr::null::<()>());
+    }
+}
+
+pub const ZPTR: *const usize = 0 as *const _;
+pub const NOT_ZPTR: *const usize = 1 as *const _;
+
+fn transmute_const() {
+    unsafe {
+        // Should raise a lint.
+        let _: fn() = std::mem::transmute(ZPTR);
+        // Should NOT raise a lint.
+        let _: fn() = std::mem::transmute(NOT_ZPTR);
+    }
+}
+
+fn main() {
+    one_liners();
+    transmute_const();
+}
diff --git a/tests/ui/transmute_null_to_fn.stderr b/tests/ui/transmute_null_to_fn.stderr
