diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/transmute/eager_transmute.rs | 74 | ||||
| -rw-r--r-- | clippy_lints/src/transmute/mod.rs | 61 | ||||
| -rw-r--r-- | tests/ui/eager_transmute.fixed | 72 | ||||
| -rw-r--r-- | tests/ui/eager_transmute.rs | 72 | ||||
| -rw-r--r-- | tests/ui/eager_transmute.stderr | 92 |
8 files changed, 373 insertions, 1 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb4eb17d4..62f1e8d5a58 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_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#eager_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 38fda36c58a..fc5b8865495 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_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/lib.rs b/clippy_lints/src/lib.rs index 61935dc7e3c..755a4ff525d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -22,6 +22,7 @@ // FIXME: switch to something more ergonomic here, once available. // (Currently there is no way to opt into sysroot crates without `extern crate`.) extern crate pulldown_cmark; +extern crate rustc_abi; extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_ast_pretty; diff --git a/clippy_lints/src/transmute/eager_transmute.rs b/clippy_lints/src/transmute/eager_transmute.rs new file mode 100644 index 00000000000..01a23c515f5 --- /dev/null +++ b/clippy_lints/src/transmute/eager_transmute.rs @@ -0,0 +1,74 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::ty::is_normalizable; +use clippy_utils::{path_to_local, path_to_local_id}; +use rustc_abi::WrappingRange; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Node}; +use rustc_lint::LateContext; +use rustc_middle::ty::Ty; + +use super::EAGER_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 +} + +fn range_fully_contained(from: WrappingRange, to: WrappingRange) -> bool { + to.contains(from.start) && to.contains(from.end) +} + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + transmutable: &'tcx Expr<'tcx>, + from_ty: Ty<'tcx>, + to_ty: Ty<'tcx>, +) -> bool { + if let Some(then_some_call) = peel_parent_unsafe_blocks(cx, expr) + && let ExprKind::MethodCall(path, receiver, [arg], _) = then_some_call.kind + && cx.typeck_results().expr_ty(receiver).is_bool() + && path.ident.name == sym!(then_some) + && let ExprKind::Binary(_, lhs, rhs) = receiver.kind + && let Some(local_id) = path_to_local(transmutable) + && (path_to_local_id(lhs, local_id) || path_to_local_id(rhs, local_id)) + && is_normalizable(cx, cx.param_env, from_ty) + && is_normalizable(cx, cx.param_env, to_ty) + // we only want to lint if the target type has a niche that is larger than the one of the source type + // e.g. `u8` to `NonZeroU8` should lint, but `NonZeroU8` to `u8` should not + && let Ok(from_layout) = cx.tcx.layout_of(cx.param_env.and(from_ty)) + && let Ok(to_layout) = cx.tcx.layout_of(cx.param_env.and(to_ty)) + && match (from_layout.largest_niche, to_layout.largest_niche) { + (Some(from_niche), Some(to_niche)) => !range_fully_contained(from_niche.valid_range, to_niche.valid_range), + (None, Some(_)) => true, + (_, None) => false, + } + { + span_lint_and_then( + cx, + EAGER_TRANSMUTE, + expr.span, + "this transmute is always evaluated eagerly, even if the condition is false", + |diag| { + diag.multipart_suggestion( + "consider using `bool::then` to only transmute if the condition holds", + vec![ + (path.ident.span, "then".into()), + (arg.span.shrink_to_lo(), "|| ".into()), + ], + Applicability::MaybeIncorrect, + ); + }, + ); + true + } else { + false + } +} diff --git a/clippy_lints/src/transmute/mod.rs b/clippy_lints/src/transmute/mod.rs index 95a92afea66..06de7a11031 100644 --- a/clippy_lints/src/transmute/mod.rs +++ b/clippy_lints/src/transmute/mod.rs @@ -1,4 +1,5 @@ mod crosspointer_transmute; +mod eager_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<Opcode> { + /// (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<Opcode> { + /// (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_TRANSMUTE, + correctness, + "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_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_transmute::check(cx, e, arg, from_ty, to_ty)); if !linted { transmutes_expressible_as_ptr_casts::check(cx, e, from_ty, from_ty_adjusted, to_ty, arg); diff --git a/tests/ui/eager_transmute.fixed b/tests/ui/eager_transmute.fixed new file mode 100644 index 00000000000..e06aa2cc9fd --- /dev/null +++ b/tests/ui/eager_transmute.fixed @@ -0,0 +1,72 @@ +#![feature(rustc_attrs)] +#![warn(clippy::eager_transmute)] +#![allow(clippy::transmute_int_to_non_zero)] + +use std::num::NonZeroU8; + +#[repr(u8)] +enum Opcode { + Add = 0, + Sub = 1, + Mul = 2, + Div = 3, +} + +fn int_to_opcode(op: u8) -> Option<Opcode> { + (op < 4).then(|| 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(|| unsafe { std::mem::transmute::<_, Opcode>(op) }); + (op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) }); + (op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) }); +} + +unsafe fn f2(op: u8) { + (op < 4).then(|| std::mem::transmute::<_, Opcode>(op)); +} + +#[rustc_layout_scalar_valid_range_end(254)] +struct NonMaxU8(u8); +#[rustc_layout_scalar_valid_range_end(254)] +#[rustc_layout_scalar_valid_range_start(1)] +struct NonZeroNonMaxU8(u8); + +macro_rules! impls { + ($($t:ty),*) => { + $( + impl PartialEq<u8> for $t { + fn eq(&self, other: &u8) -> bool { + self.0 == *other + } + } + impl PartialOrd<u8> for $t { + fn partial_cmp(&self, other: &u8) -> Option<std::cmp::Ordering> { + self.0.partial_cmp(other) + } + } + )* + }; +} +impls!(NonMaxU8, NonZeroNonMaxU8); + +fn niche_tests(v1: u8, v2: NonZeroU8, v3: NonZeroNonMaxU8) { + // u8 -> NonZeroU8, do lint + let _: Option<NonZeroU8> = (v1 > 0).then(|| unsafe { std::mem::transmute(v1) }); + + // NonZeroU8 -> u8, don't lint, target type has no niche and therefore a higher validity range + let _: Option<u8> = (v2 > NonZeroU8::new(1).unwrap()).then_some(unsafe { std::mem::transmute(v2) }); + + // NonZeroU8 -> NonMaxU8, do lint, different niche + let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) }); + + // NonZeroNonMaxU8 -> NonMaxU8, don't lint, target type has more validity + let _: Option<NonMaxU8> = (v3 < 255).then_some(unsafe { std::mem::transmute(v2) }); + + // NonZeroU8 -> NonZeroNonMaxU8, do lint, target type has less validity + let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) }); +} + +fn main() {} diff --git a/tests/ui/eager_transmute.rs b/tests/ui/eager_transmute.rs new file mode 100644 index 00000000000..89ccdf583f3 --- /dev/null +++ b/tests/ui/eager_transmute.rs @@ -0,0 +1,72 @@ +#![feature(rustc_attrs)] +#![warn(clippy::eager_transmute)] +#![allow(clippy::transmute_int_to_non_zero)] + +use std::num::NonZeroU8; + +#[repr(u8)] +enum Opcode { + Add = 0, + Sub = 1, + Mul = 2, + Div = 3, +} + +fn int_to_opcode(op: u8) -> Option<Opcode> { + (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)); +} + +#[rustc_layout_scalar_valid_range_end(254)] +struct NonMaxU8(u8); +#[rustc_layout_scalar_valid_range_end(254)] +#[rustc_layout_scalar_valid_range_start(1)] +struct NonZeroNonMaxU8(u8); + +macro_rules! impls { + ($($t:ty),*) => { + $( + impl PartialEq<u8> for $t { + fn eq(&self, other: &u8) -> bool { + self.0 == *other + } + } + impl PartialOrd<u8> for $t { + fn partial_cmp(&self, other: &u8) -> Option<std::cmp::Ordering> { + self.0.partial_cmp(other) + } + } + )* + }; +} +impls!(NonMaxU8, NonZeroNonMaxU8); + +fn niche_tests(v1: u8, v2: NonZeroU8, v3: NonZeroNonMaxU8) { + // u8 -> NonZeroU8, do lint + let _: Option<NonZeroU8> = (v1 > 0).then_some(unsafe { std::mem::transmute(v1) }); + + // NonZeroU8 -> u8, don't lint, target type has no niche and therefore a higher validity range + let _: Option<u8> = (v2 > NonZeroU8::new(1).unwrap()).then_some(unsafe { std::mem::transmute(v2) }); + + // NonZeroU8 -> NonMaxU8, do lint, different niche + let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) }); + + // NonZeroNonMaxU8 -> NonMaxU8, don't lint, target type has more validity + let _: Option<NonMaxU8> = (v3 < 255).then_some(unsafe { std::mem::transmute(v2) }); + + // NonZeroU8 -> NonZeroNonMaxU8, do lint, target type has less validity + let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) }); +} + +fn main() {} diff --git a/tests/ui/eager_transmute.stderr b/tests/ui/eager_transmute.stderr new file mode 100644 index 00000000000..5eb163c5fcb --- /dev/null +++ b/tests/ui/eager_transmute.stderr @@ -0,0 +1,92 @@ +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:16:33 + | +LL | (op < 4).then_some(unsafe { std::mem::transmute(op) }) + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::eager-transmute` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::eager_transmute)]` +help: consider using `bool::then` to only transmute if the condition holds + | +LL | (op < 4).then(|| unsafe { std::mem::transmute(op) }) + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:22:33 + | +LL | (op < 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | (op < 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:23:33 + | +LL | (op > 4).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | (op > 4).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:24:34 + | +LL | (op == 0).then_some(unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | (op == 0).then(|| unsafe { std::mem::transmute::<_, Opcode>(op) }); + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:28:24 + | +LL | (op < 4).then_some(std::mem::transmute::<_, Opcode>(op)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | (op < 4).then(|| std::mem::transmute::<_, Opcode>(op)); + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:57:60 + | +LL | let _: Option<NonZeroU8> = (v1 > 0).then_some(unsafe { std::mem::transmute(v1) }); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | let _: Option<NonZeroU8> = (v1 > 0).then(|| unsafe { std::mem::transmute(v1) }); + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:63:86 + | +LL | let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) }); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | let _: Option<NonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) }); + | ~~~~ ++ + +error: this transmute is always evaluated eagerly, even if the condition is false + --> $DIR/eager_transmute.rs:69:93 + | +LL | let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then_some(unsafe { std::mem::transmute(v2) }); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `bool::then` to only transmute if the condition holds + | +LL | let _: Option<NonZeroNonMaxU8> = (v2 < NonZeroU8::new(255).unwrap()).then(|| unsafe { std::mem::transmute(v2) }); + | ~~~~ ++ + +error: aborting due to 8 previous errors + |