diff --git a/CHANGELOG.md b/CHANGELOG.md index 903ee938d9d..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 @@ -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 index 3cd7d1d7e72..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, @@ -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 new file mode 100644 index 00000000000..91c8c340ce2 --- /dev/null +++ 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 = /* 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` 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 * ).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 * ) == (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/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_lints/src/transmute/mod.rs b/clippy_lints/src/transmute/mod.rs index 83e651aba8e..691d759d773 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..e75d7f6bf1d --- /dev/null +++ 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` 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::())` + 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 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; 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, } 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 + 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 +