diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb4eb17d46..c23a77b74831 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5044,6 +5044,7 @@ Released 2018-09-13 [`duplicate_mod`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_mod [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument [`duration_subsec`]: https://rust-lang.github.io/rust-clippy/master/index.html#duration_subsec +[`eager_int_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#eager_int_transmute [`else_if_without_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#else_if_without_else [`empty_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_drop [`empty_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#empty_enum diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 38fda36c58ac..198ebab5517a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -651,6 +651,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO, crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO, crate::transmute::CROSSPOINTER_TRANSMUTE_INFO, + crate::transmute::EAGER_INT_TRANSMUTE_INFO, crate::transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS_INFO, crate::transmute::TRANSMUTE_BYTES_TO_STR_INFO, crate::transmute::TRANSMUTE_FLOAT_TO_INT_INFO, diff --git a/clippy_lints/src/transmute/eager_int_transmute.rs b/clippy_lints/src/transmute/eager_int_transmute.rs new file mode 100644 index 000000000000..67ec4510cb1a --- /dev/null +++ b/clippy_lints/src/transmute/eager_int_transmute.rs @@ -0,0 +1,39 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::{path_to_local, path_to_local_id}; +use rustc_hir::{Expr, ExprKind, Node}; +use rustc_lint::LateContext; + +use super::EAGER_INT_TRANSMUTE; + +fn peel_parent_unsafe_blocks<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + for (_, parent) in cx.tcx.hir().parent_iter(expr.hir_id) { + match parent { + Node::Block(_) => {}, + Node::Expr(e) if let ExprKind::Block(..) = e.kind => {}, + Node::Expr(e) => return Some(e), + _ => break, + } + } + None +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, transmutable: &'tcx Expr<'tcx>) -> bool { + if let Some(then_some_call) = peel_parent_unsafe_blocks(cx, expr) + && let ExprKind::MethodCall(path, receiver, ..) = then_some_call.kind + && cx.typeck_results().expr_ty(receiver).is_bool() + && path.ident.name == sym!(then_some) + && let ExprKind::Binary(_, lhs, _) = receiver.kind + && let Some(local_id) = path_to_local(transmutable) + && path_to_local_id(lhs, local_id) + { + span_lint_and_help( + cx, + EAGER_INT_TRANSMUTE, + expr.span, + "this transmute is evaluated eagerly, even if the condition is false", + Some(path.ident.span), + "consider using `then` instead, which evaluates it only if the condition is true", + ); + } + false +} diff --git a/clippy_lints/src/transmute/mod.rs b/clippy_lints/src/transmute/mod.rs index 95a92afea668..8a244ad674f1 100644 --- a/clippy_lints/src/transmute/mod.rs +++ b/clippy_lints/src/transmute/mod.rs @@ -1,4 +1,5 @@ mod crosspointer_transmute; +mod eager_int_transmute; mod transmute_float_to_int; mod transmute_int_to_bool; mod transmute_int_to_char; @@ -463,6 +464,62 @@ declare_clippy_lint! { "transmute results in a null function pointer, which is undefined behavior" } +declare_clippy_lint! { + /// ### What it does + /// Checks for integer validity checks, followed by a transmute that is (incorrectly) evaluated + /// eagerly (e.g. using `bool::then_some`). + /// + /// ### Why is this bad? + /// Eager evaluation means that the `transmute` call is executed regardless of whether the condition is true or false. + /// This can introduce unsoundness and other subtle bugs. + /// + /// ### Example + /// Consider the following function which is meant to convert an unsigned integer to its enum equivalent via transmute. + /// + /// ```no_run + /// #[repr(u8)] + /// enum Opcode { + /// Add = 0, + /// Sub = 1, + /// Mul = 2, + /// Div = 3 + /// } + /// + /// fn int_to_opcode(op: u8) -> Option { + /// (op < 4).then_some(unsafe { std::mem::transmute(op) }) + /// } + /// ``` + /// This may appear fine at first given that it checks that the `u8` is within the validity range of the enum, + /// *however* the transmute is evaluated eagerly, meaning that it executes even if `op >= 4`! + /// + /// This makes the function unsound, because it is possible for the caller to cause undefined behavior + /// (creating an enum with an invalid bitpattern) entirely in safe code only by passing an incorrect value, + /// which is normally only a bug that is possible in unsafe code. + /// + /// One possible way in which this can go wrong practically is that the compiler sees it as: + /// ```rust,ignore (illustrative) + /// let temp: Foo = unsafe { std::mem::transmute(op) }; + /// (0 < 4).then_some(temp) + /// ``` + /// and optimizes away the `(0 < 4)` check based on the assumption that since a `Foo` was created from `op` with the validity range `0..3`, + /// it is **impossible** for this condition to be false. + /// + /// In short, it is possible for this function to be optimized in a way that makes it [never return `None`](https://godbolt.org/z/ocrcenevq), + /// even if passed the value `4`. + /// + /// This can be avoided by instead using lazy evaluation. For the example above, this should be written: + /// ```rust,ignore (illustrative) + /// fn int_to_opcode(op: u8) -> Option { + /// (op < 4).then(|| unsafe { std::mem::transmute(op) }) + /// ^^^^ ^^ `bool::then` only executes the closure if the condition is true! + /// } + /// ``` + #[clippy::version = "1.76.0"] + pub EAGER_INT_TRANSMUTE, + suspicious, + "eager evaluation of `transmute`" +} + pub struct Transmute { msrv: Msrv, } @@ -484,6 +541,7 @@ impl_lint_pass!(Transmute => [ TRANSMUTE_UNDEFINED_REPR, TRANSMUTING_NULL, TRANSMUTE_NULL_TO_FN, + EAGER_INT_TRANSMUTE, ]); impl Transmute { #[must_use] @@ -530,7 +588,8 @@ impl<'tcx> LateLintPass<'tcx> for Transmute { | transmute_float_to_int::check(cx, e, from_ty, to_ty, arg, const_context) | transmute_num_to_bytes::check(cx, e, from_ty, to_ty, arg, const_context) | (unsound_collection_transmute::check(cx, e, from_ty, to_ty) - || transmute_undefined_repr::check(cx, e, from_ty, to_ty)); + || transmute_undefined_repr::check(cx, e, from_ty, to_ty)) + | (eager_int_transmute::check(cx, e, arg)); if !linted { transmutes_expressible_as_ptr_casts::check(cx, e, from_ty, from_ty_adjusted, to_ty, arg); diff --git a/tests/ui/eager_int_transmute.rs b/tests/ui/eager_int_transmute.rs new file mode 100644 index 000000000000..642169f91d04 --- /dev/null +++ b/tests/ui/eager_int_transmute.rs @@ -0,0 +1,27 @@ +#![warn(clippy::eager_int_transmute)] + +#[repr(u8)] +enum Opcode { + Add = 0, + Sub = 1, + Mul = 2, + Div = 3, +} + +fn int_to_opcode(op: u8) -> Option { + (op < 4).then_some(unsafe { std::mem::transmute(op) }) +} + +fn f(op: u8, unrelated: u8) { + true.then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + (unrelated < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); +} + +unsafe fn f2(op: u8) { + (op < 4).then_some(std::mem::transmute::<_, Opcode>(op)); +} + +fn main() {} diff --git a/tests/ui/eager_int_transmute.stderr b/tests/ui/eager_int_transmute.stderr new file mode 100644 index 000000000000..e35c1e3fcd4f --- /dev/null +++ b/tests/ui/eager_int_transmute.stderr @@ -0,0 +1,64 @@ +error: this transmute is evaluated eagerly, even if the condition is false + --> $DIR/eager_int_transmute.rs:12:33 + | +LL | (op < 4).then_some(unsafe { std::mem::transmute(op) }) + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `then` instead, which evaluates it only if the condition is true + --> $DIR/eager_int_transmute.rs:12:14 + | +LL | (op < 4).then_some(unsafe { std::mem::transmute(op) }) + | ^^^^^^^^^ + = note: `-D clippy::eager-int-transmute` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::eager_int_transmute)]` + +error: this transmute is evaluated eagerly, even if the condition is false + --> $DIR/eager_int_transmute.rs:18:33 + | +LL | (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `then` instead, which evaluates it only if the condition is true + --> $DIR/eager_int_transmute.rs:18:14 + | +LL | (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^ + +error: this transmute is evaluated eagerly, even if the condition is false + --> $DIR/eager_int_transmute.rs:19:33 + | +LL | (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `then` instead, which evaluates it only if the condition is true + --> $DIR/eager_int_transmute.rs:19:14 + | +LL | (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^ + +error: this transmute is evaluated eagerly, even if the condition is false + --> $DIR/eager_int_transmute.rs:20:34 + | +LL | (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `then` instead, which evaluates it only if the condition is true + --> $DIR/eager_int_transmute.rs:20:15 + | +LL | (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^ + +error: this transmute is evaluated eagerly, even if the condition is false + --> $DIR/eager_int_transmute.rs:24:24 + | +LL | (op < 4).then_some(std::mem::transmute::<_, Opcode>(op)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `then` instead, which evaluates it only if the condition is true + --> $DIR/eager_int_transmute.rs:24:14 + | +LL | (op < 4).then_some(std::mem::transmute::<_, Opcode>(op)); + | ^^^^^^^^^ + +error: aborting due to 5 previous errors +