From 6afe5471cf3cc78b5ce85d1bbb13a010516c500e Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sat, 17 Dec 2022 20:22:52 +0300 Subject: [PATCH 01/10] Add lint `transmute_null_to_fn` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/transmute/mod.rs | 31 ++++++++++ .../src/transmute/transmute_null_to_fn.rs | 62 +++++++++++++++++++ tests/ui/transmute_null_to_fn.rs | 28 +++++++++ tests/ui/transmute_null_to_fn.stderr | 27 ++++++++ 6 files changed, 150 insertions(+) create mode 100644 clippy_lints/src/transmute/transmute_null_to_fn.rs create mode 100644 tests/ui/transmute_null_to_fn.rs create mode 100644 tests/ui/transmute_null_to_fn.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 903ee938d9d..3c2801cfb5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4590,6 +4590,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 index 3cd7d1d7e72..5bae62ce24f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -568,6 +568,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/transmute/mod.rs b/clippy_lints/src/transmute/mod.rs index 83e651aba8e..7a2ab2bb4c4 100644 --- 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 = 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 new file mode 100644 index 00000000000..db89078f657 --- /dev/null +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -0,0 +1,62 @@ +use clippy_utils::consts::{constant_context, 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; + +const LINT_MSG: &str = "transmuting a known null pointer into a function pointer"; +const NOTE_MSG: &str = "this transmute results in a null function pointer"; +const HELP_MSG: &str = + "try wrapping your function pointer type in `Option` 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; + } + + // 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 + { + span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { + diag.span_label(expr.span, NOTE_MSG); + diag.help(HELP_MSG); + }); + return true; + } + + // Catching: + // `std::mem::transmute(0 as *const i32)` + if let ExprKind::Cast(inner_expr, _cast_ty) = arg.kind && is_integer_literal(inner_expr, 0) { + span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { + diag.span_label(expr.span, NOTE_MSG); + diag.help(HELP_MSG); + }); + return true; + } + + // Catching: + // `std::mem::transmute(std::ptr::null::())` + if let ExprKind::Call(func1, []) = arg.kind && + is_path_diagnostic_item(cx, func1, sym::ptr_null) + { + span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { + diag.span_label(expr.span, NOTE_MSG); + diag.help(HELP_MSG); + }); + return 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/tests/ui/transmute_null_to_fn.rs b/tests/ui/transmute_null_to_fn.rs new file mode 100644 index 00000000000..b3ea3d9039e --- /dev/null +++ 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 new file mode 100644 index 00000000000..f0c65497d75 --- /dev/null +++ b/tests/ui/transmute_null_to_fn.stderr @@ -0,0 +1,27 @@ +error: transmuting a known null pointer into a function pointer + --> $DIR/transmute_null_to_fn.rs:8:23 + | +LL | let _: fn() = std::mem::transmute(0 as *const ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this transmute results in undefined behavior + | + = help: try wrapping your function pointer type in `Option` instead, and using `None` as a null pointer value + = note: `-D clippy::transmute-null-to-fn` implied by `-D warnings` + +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` instead, and using `None` as a null pointer value + +error: transmuting a known null pointer into a function pointer + --> $DIR/transmute_null_to_fn.rs:19:23 + | +LL | let _: fn() = std::mem::transmute(ZPTR); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ this transmute results in undefined behavior + | + = help: try wrapping your function pointer type in `Option` instead, and using `None` as a null pointer value + +error: aborting due to 3 previous errors + From 3cc67d08569b5cf847242532f99c5aca838dbdeb Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 02:14:09 +0300 Subject: [PATCH 02/10] Relax clippy_utils::consts::miri_to_const pointer type restrictiveness --- clippy_lints/src/lib.rs | 2 ++ clippy_utils/src/consts.rs | 7 +------ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 39850d59803..7f7d73339e4 100644 --- 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_utils/src/consts.rs b/clippy_utils/src/consts.rs index 7a637d32bab..a67bd8d4600 100644 --- 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, } From 42106e04951f409c88131a42a0514af479ecb932 Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 02:16:04 +0300 Subject: [PATCH 03/10] Add lint `fn_null_check` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/fn_null_check.rs | 129 ++++++++++++++++++ .../src/transmute/transmute_null_to_fn.rs | 2 +- tests/ui/fn_null_check.rs | 21 +++ tests/ui/fn_null_check.stderr | 43 ++++++ 6 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/fn_null_check.rs create mode 100644 tests/ui/fn_null_check.rs create mode 100644 tests/ui/fn_null_check.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c2801cfb5e..17ff182c7be 100644 --- 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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5bae62ce24f..480a65ac70c 100644 --- 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, diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs new file mode 100644 index 00000000000..d0e82500dd0 --- /dev/null +++ b/clippy_lints/src/fn_null_check.rs @@ -0,0 +1,129 @@ +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 if_chain::if_chain; +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 + /// let fn_ptr: Option = /* 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]); + +const LINT_MSG: &str = "function pointer assumed to be nullable, even though it isn't"; +const HELP_MSG: &str = "try wrapping your function pointer type in `Option` instead, and using `is_none` to check for null pointer value"; + +fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if_chain! { + if let ExprKind::Cast(cast_expr, cast_ty) = expr.kind; + if let TyKind::Ptr(_) = cast_ty.kind; + if cx.typeck_results().expr_ty_adjusted(cast_expr).is_fn(); + then { + true + } else { + false + } + } +} + +impl<'tcx> LateLintPass<'tcx> for FnNullCheck { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + // Catching: + // (fn_ptr as * ).is_null() + if_chain! { + if let ExprKind::MethodCall(method_name, receiver, _, _) = expr.kind; + if method_name.ident.as_str() == "is_null"; + if is_fn_ptr_cast(cx, receiver); + then { + span_lint_and_help( + cx, + FN_NULL_CHECK, + expr.span, + LINT_MSG, + None, + HELP_MSG + ); + } + } + + if let ExprKind::Binary(op, left, right) = expr.kind + && let BinOpKind::Eq = op.node + { + 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; + } + + // Catching: + // (fn_ptr as * ) == + let c = constant(cx, cx.typeck_results(), to_check); + if let Some((Constant::RawPtr(0), _)) = c { + span_lint_and_help( + cx, + FN_NULL_CHECK, + expr.span, + LINT_MSG, + None, + HELP_MSG + ); + return; + } + + // Catching: + // (fn_ptr as * ) == (0 as ) + if let ExprKind::Cast(cast_expr, _) = to_check.kind && is_integer_literal(cast_expr, 0) { + span_lint_and_help( + cx, + FN_NULL_CHECK, + expr.span, + LINT_MSG, + None, + HELP_MSG + ); + return; + } + + // Catching: + // (fn_ptr as * ) == std::ptr::null() + if let ExprKind::Call(func, []) = to_check.kind && + is_path_diagnostic_item(cx, func, sym::ptr_null) + { + span_lint_and_help( + cx, + FN_NULL_CHECK, + expr.span, + LINT_MSG, + None, + HELP_MSG + ); + } + } + } +} diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs index db89078f657..032b4c1e2d2 100644 --- a/clippy_lints/src/transmute/transmute_null_to_fn.rs +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -9,7 +9,7 @@ use rustc_span::symbol::sym; use super::TRANSMUTE_NULL_TO_FN; const LINT_MSG: &str = "transmuting a known null pointer into a function pointer"; -const NOTE_MSG: &str = "this transmute results in a null function pointer"; +const NOTE_MSG: &str = "this transmute results in undefined behavior"; const HELP_MSG: &str = "try wrapping your function pointer type in `Option` instead, and using `None` as a null pointer value"; diff --git a/tests/ui/fn_null_check.rs b/tests/ui/fn_null_check.rs new file mode 100644 index 00000000000..df5bc8420d5 --- /dev/null +++ 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 new file mode 100644 index 00000000000..660dd323979 --- /dev/null +++ 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` 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` 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` 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` 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` instead, and using `is_none` to check for null pointer value + +error: aborting due to 5 previous errors + From 54a9168efb9d1a34e8430199d20a7fd5db032e7b Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 02:25:21 +0300 Subject: [PATCH 04/10] Remove useless pattern matching --- clippy_lints/src/transmute/transmute_null_to_fn.rs | 3 +-- clippy_lints/src/transmute/transmuting_null.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs index 032b4c1e2d2..2286e17fd16 100644 --- a/clippy_lints/src/transmute/transmute_null_to_fn.rs +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -21,8 +21,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_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { diag.span_label(expr.span, NOTE_MSG); diff --git a/clippy_lints/src/transmute/transmuting_null.rs b/clippy_lints/src/transmute/transmuting_null.rs index 19ce5ae72c2..1e407fc4138 100644 --- 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; From dae54fad3ee4b51ed286d4686f6d064b48b56cca Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 03:20:04 +0300 Subject: [PATCH 05/10] Doc codeblock fixup --- clippy_lints/src/fn_null_check.rs | 2 +- clippy_lints/src/transmute/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs index d0e82500dd0..89841936954 100644 --- a/clippy_lints/src/fn_null_check.rs +++ b/clippy_lints/src/fn_null_check.rs @@ -21,7 +21,7 @@ declare_clippy_lint! { /// if (fn_ptr as *const ()).is_null() { ... } /// ``` /// Use instead: - /// ```rust + /// ```rust,ignore /// let fn_ptr: Option = /* somehow obtained nullable function pointer */ /// /// if fn_ptr.is_none() { ... } diff --git a/clippy_lints/src/transmute/mod.rs b/clippy_lints/src/transmute/mod.rs index 7a2ab2bb4c4..691d759d773 100644 --- a/clippy_lints/src/transmute/mod.rs +++ b/clippy_lints/src/transmute/mod.rs @@ -426,7 +426,7 @@ declare_clippy_lint! { /// /// ### Example /// ```rust - /// let null_fn: fn() = unsafe { std::mem::transmute( std::ptr::null() ) }; + /// let null_fn: fn() = unsafe { std::mem::transmute( std::ptr::null::<()>() ) }; /// ``` /// Use instead: /// ```rust From b1ca307168b622d66a7f0d66421933236bb8cbcf Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 18:58:15 +0300 Subject: [PATCH 06/10] Address some of the code style issues --- clippy_lints/src/fn_null_check.rs | 75 +++++++------------ .../src/transmute/transmute_null_to_fn.rs | 3 +- 2 files changed, 27 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs index 89841936954..f4b7a55fa74 100644 --- a/clippy_lints/src/fn_null_check.rs +++ b/clippy_lints/src/fn_null_check.rs @@ -1,7 +1,6 @@ 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 if_chain::if_chain; use rustc_hir::{BinOpKind, Expr, ExprKind, TyKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -33,19 +32,24 @@ declare_clippy_lint! { } declare_lint_pass!(FnNullCheck => [FN_NULL_CHECK]); -const LINT_MSG: &str = "function pointer assumed to be nullable, even though it isn't"; -const HELP_MSG: &str = "try wrapping your function pointer type in `Option` instead, and using `is_none` to check for null pointer value"; +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` instead, and using `is_none` to check for null pointer value", + ) +} fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if_chain! { - if let ExprKind::Cast(cast_expr, cast_ty) = expr.kind; - if let TyKind::Ptr(_) = cast_ty.kind; - if cx.typeck_results().expr_ty_adjusted(cast_expr).is_fn(); - then { - true - } else { - false - } + 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 } } @@ -53,20 +57,12 @@ impl<'tcx> LateLintPass<'tcx> for FnNullCheck { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // Catching: // (fn_ptr as * ).is_null() - if_chain! { - if let ExprKind::MethodCall(method_name, receiver, _, _) = expr.kind; - if method_name.ident.as_str() == "is_null"; - if is_fn_ptr_cast(cx, receiver); - then { - span_lint_and_help( - cx, - FN_NULL_CHECK, - expr.span, - LINT_MSG, - None, - HELP_MSG - ); - } + if let ExprKind::MethodCall(method_name, receiver, _, _) = expr.kind + && method_name.ident.as_str() == "is_null" + && is_fn_ptr_cast(cx, receiver) + { + lint_expr(cx, expr); + return; } if let ExprKind::Binary(op, left, right) = expr.kind @@ -85,28 +81,14 @@ impl<'tcx> LateLintPass<'tcx> for FnNullCheck { // (fn_ptr as * ) == let c = constant(cx, cx.typeck_results(), to_check); if let Some((Constant::RawPtr(0), _)) = c { - span_lint_and_help( - cx, - FN_NULL_CHECK, - expr.span, - LINT_MSG, - None, - HELP_MSG - ); + lint_expr(cx, expr); return; } // Catching: // (fn_ptr as * ) == (0 as ) if let ExprKind::Cast(cast_expr, _) = to_check.kind && is_integer_literal(cast_expr, 0) { - span_lint_and_help( - cx, - FN_NULL_CHECK, - expr.span, - LINT_MSG, - None, - HELP_MSG - ); + lint_expr(cx, expr); return; } @@ -115,14 +97,7 @@ impl<'tcx> LateLintPass<'tcx> for FnNullCheck { if let ExprKind::Call(func, []) = to_check.kind && is_path_diagnostic_item(cx, func, sym::ptr_null) { - span_lint_and_help( - cx, - FN_NULL_CHECK, - expr.span, - LINT_MSG, - None, - HELP_MSG - ); + lint_expr(cx, expr); } } } diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs index 2286e17fd16..074a5d31763 100644 --- a/clippy_lints/src/transmute/transmute_null_to_fn.rs +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -18,7 +18,8 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arg: &'t return false; } - // Catching transmute over constants that resolve to `null`. + // 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(0)) = const_eval_context.expr(arg) From 9b2fc8e2a2da497344d42ccf8ddefc794eb82858 Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 19:43:26 +0300 Subject: [PATCH 07/10] Make clippy happy --- clippy_lints/src/fn_null_check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs index f4b7a55fa74..6eb5ddd94d7 100644 --- a/clippy_lints/src/fn_null_check.rs +++ b/clippy_lints/src/fn_null_check.rs @@ -40,7 +40,7 @@ fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) { "function pointer assumed to be nullable, even though it isn't", None, "try wrapping your function pointer type in `Option` instead, and using `is_none` to check for null pointer value", - ) + ); } fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { From 20f501a9d9f253bfe503388755bdebfe72910e0e Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 22:39:06 +0300 Subject: [PATCH 08/10] Improve code style further --- clippy_lints/src/fn_null_check.rs | 84 +++++++++---------- .../src/transmute/transmute_null_to_fn.rs | 69 ++++++++------- 2 files changed, 76 insertions(+), 77 deletions(-) diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs index 6eb5ddd94d7..4f79ce6f8fe 100644 --- a/clippy_lints/src/fn_null_check.rs +++ b/clippy_lints/src/fn_null_check.rs @@ -55,50 +55,50 @@ fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { impl<'tcx> LateLintPass<'tcx> for FnNullCheck { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - // Catching: - // (fn_ptr as * ).is_null() - if let ExprKind::MethodCall(method_name, receiver, _, _) = expr.kind - && method_name.ident.as_str() == "is_null" - && is_fn_ptr_cast(cx, receiver) - { - lint_expr(cx, expr); - return; - } - - if let ExprKind::Binary(op, left, right) = expr.kind - && let BinOpKind::Eq = op.node - { - 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; - } - - // Catching: - // (fn_ptr as * ) == - let c = constant(cx, cx.typeck_results(), to_check); - if let Some((Constant::RawPtr(0), _)) = c { - lint_expr(cx, expr); - return; - } - - // Catching: - // (fn_ptr as * ) == (0 as ) - if let ExprKind::Cast(cast_expr, _) = to_check.kind && is_integer_literal(cast_expr, 0) { - lint_expr(cx, expr); - return; - } - - // Catching: - // (fn_ptr as * ) == std::ptr::null() - if let ExprKind::Call(func, []) = to_check.kind && - is_path_diagnostic_item(cx, func, sym::ptr_null) + match expr.kind { + 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 * ) == (0 as ) + ExprKind::Cast(cast_expr, _) if is_integer_literal(cast_expr, 0) => { + lint_expr(cx, expr); + }, + + // Catching: + // (fn_ptr as * ) == std::ptr::null() + ExprKind::Call(func, []) if is_path_diagnostic_item(cx, func, sym::ptr_null) => { + lint_expr(cx, expr); + }, + + // Catching: + // (fn_ptr as * ) == + _ if matches!( + constant(cx, cx.typeck_results(), to_check), + Some((Constant::RawPtr(0), _)) + ) => + { + lint_expr(cx, expr); + }, + + _ => {}, + } + }, + _ => {}, } } } diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs index 074a5d31763..79d8cb084fd 100644 --- a/clippy_lints/src/transmute/transmute_null_to_fn.rs +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -13,6 +13,13 @@ const NOTE_MSG: &str = "this transmute results in undefined behavior"; const HELP_MSG: &str = "try wrapping your function pointer type in `Option` instead, and using `None` as a null pointer value"; +fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) { + span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { + diag.span_label(expr.span, NOTE_MSG); + diag.help(HELP_MSG); + }); +} + 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; @@ -21,42 +28,34 @@ 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(0)) = const_eval_context.expr(arg) - { - span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { - diag.span_label(expr.span, NOTE_MSG); - diag.help(HELP_MSG); - }); - return true; - } - // Catching: - // `std::mem::transmute(0 as *const i32)` - if let ExprKind::Cast(inner_expr, _cast_ty) = arg.kind && is_integer_literal(inner_expr, 0) { - span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { - diag.span_label(expr.span, NOTE_MSG); - diag.help(HELP_MSG); - }); - return true; - } + match arg.kind { + ExprKind::Path(ref _qpath) if matches!(const_eval_context.expr(arg), Some(Constant::RawPtr(0))) => { + lint_expr(cx, expr); + true + }, - // Catching: - // `std::mem::transmute(std::ptr::null::())` - if let ExprKind::Call(func1, []) = arg.kind && - is_path_diagnostic_item(cx, func1, sym::ptr_null) - { - span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { - diag.span_label(expr.span, NOTE_MSG); - diag.help(HELP_MSG); - }); - return 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 + }, - // 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 + // Catching: + // `std::mem::transmute(std::ptr::null::())` + 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 + }, + } } From cc985748838c64b7bf4a3be635026bfffb02088c Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Sun, 18 Dec 2022 22:47:02 +0300 Subject: [PATCH 09/10] Fix comments, use `constant` instead of raw `constant_context` --- clippy_lints/src/fn_null_check.rs | 2 ++ clippy_lints/src/transmute/transmute_null_to_fn.rs | 12 ++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/fn_null_check.rs b/clippy_lints/src/fn_null_check.rs index 4f79ce6f8fe..91c8c340ce2 100644 --- a/clippy_lints/src/fn_null_check.rs +++ b/clippy_lints/src/fn_null_check.rs @@ -56,6 +56,8 @@ fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { impl<'tcx> LateLintPass<'tcx> for FnNullCheck { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { match expr.kind { + // Catching: + // (fn_ptr as * ).is_null() ExprKind::MethodCall(method_name, receiver, _, _) if method_name.ident.as_str() == "is_null" && is_fn_ptr_cast(cx, receiver) => { diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs index 79d8cb084fd..410c9eaaba0 100644 --- a/clippy_lints/src/transmute/transmute_null_to_fn.rs +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -1,4 +1,4 @@ -use clippy_utils::consts::{constant_context, Constant}; +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}; @@ -25,12 +25,12 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arg: &'t return false; } - // Catching: - // transmute over constants that resolve to `null`. - let mut const_eval_context = constant_context(cx, cx.typeck_results()); - match arg.kind { - ExprKind::Path(ref _qpath) if matches!(const_eval_context.expr(arg), Some(Constant::RawPtr(0))) => { + // 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 }, From 691df70bbcab3a22a961fe4df839857af19f8413 Mon Sep 17 00:00:00 2001 From: Niki4tap Date: Mon, 19 Dec 2022 15:32:49 +0300 Subject: [PATCH 10/10] Inline some `const`s --- .../src/transmute/transmute_null_to_fn.rs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/transmute/transmute_null_to_fn.rs b/clippy_lints/src/transmute/transmute_null_to_fn.rs index 410c9eaaba0..e75d7f6bf1d 100644 --- a/clippy_lints/src/transmute/transmute_null_to_fn.rs +++ b/clippy_lints/src/transmute/transmute_null_to_fn.rs @@ -8,16 +8,19 @@ use rustc_span::symbol::sym; use super::TRANSMUTE_NULL_TO_FN; -const LINT_MSG: &str = "transmuting a known null pointer into a function pointer"; -const NOTE_MSG: &str = "this transmute results in undefined behavior"; -const HELP_MSG: &str = - "try wrapping your function pointer type in `Option` instead, and using `None` as a null pointer value"; - fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) { - span_lint_and_then(cx, TRANSMUTE_NULL_TO_FN, expr.span, LINT_MSG, |diag| { - diag.span_label(expr.span, NOTE_MSG); - diag.help(HELP_MSG); - }); + 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` 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 {