diff --git a/CHANGELOG.md b/CHANGELOG.md index ee6b3c66e6ec..6322c7a72f93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7406,6 +7406,7 @@ Released 2018-09-13 [`trim_split_whitespace`]: https://rust-lang.github.io/rust-clippy/master/index.html#trim_split_whitespace [`trivial_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_regex [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref +[`trusted_asref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trusted_asref [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err [`tuple_array_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#tuple_array_conversions [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 51a848d022d8..b9c7b253c0be 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -752,6 +752,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::transmute::UNSOUND_COLLECTION_TRANSMUTE_INFO, crate::transmute::USELESS_TRANSMUTE_INFO, crate::transmute::WRONG_TRANSMUTE_INFO, + crate::trusted_asref::TRUSTED_ASREF_INFO, crate::tuple_array_conversions::TUPLE_ARRAY_CONVERSIONS_INFO, crate::types::BORROWED_BOX_INFO, crate::types::BOX_COLLECTION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 600b37c4204c..f661d3baec3c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -363,6 +363,7 @@ mod toplevel_ref_arg; mod trailing_empty_array; mod trait_bounds; mod transmute; +mod trusted_asref; mod tuple_array_conversions; mod types; mod unconditional_recursion; @@ -862,6 +863,7 @@ rustc_lint::late_lint_methods!( ManualAssertEq: manual_assert_eq::ManualAssertEq = manual_assert_eq::ManualAssertEq, WithCapacityZero: with_capacity_zero::WithCapacityZero = with_capacity_zero::WithCapacityZero, RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns, + TrustedAsref: trusted_asref::TrustedAsref = trusted_asref::TrustedAsref, // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/clippy_lints/src/trusted_asref.rs b/clippy_lints/src/trusted_asref.rs new file mode 100644 index 000000000000..c9165a1afa96 --- /dev/null +++ b/clippy_lints/src/trusted_asref.rs @@ -0,0 +1,140 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::res::MaybeDef; +use clippy_utils::{SpanlessEq, hash_expr}; +use rustc_data_structures::fx::FxIndexMap; +use rustc_hir as hir; +use rustc_hir::intravisit::{Visitor, walk_expr}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::declare_lint_pass; +use rustc_span::{Span, sym}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for unsafe code calling `.as_ref()` multiple times + /// on the same generic receiver. + /// + /// ### Why is this bad? + /// Usually `AsRef::as_ref()` is pure and multiple calls will return a + /// reference to the exact same value. However, unsafe code cannot always + /// soundly rely on this. + /// + /// ### Example + /// ```no_run + /// # unsafe fn ffi_call(_ptr: *const u8, _len: usize) {} + /// fn safe_ffi_call(data: impl AsRef<[u8]>) { + /// unsafe { + /// ffi_call(data.as_ref().as_ptr(), data.as_ref().len()); + /// } + /// } + /// ``` + /// Use instead: + /// ```no_run + /// # unsafe fn ffi_call(_ptr: *const u8, _len: usize) {} + /// fn safe_ffi_call(data: impl AsRef<[u8]>) { + /// let data = data.as_ref(); + /// unsafe { + /// ffi_call(data.as_ptr(), data.len()); + /// } + /// } + /// ``` + /// + /// ### Known problems + /// False negatives: Such code can also be unsound when the receiver + /// is a concrete type from a foreign crate. + /// + /// False positives: Not all generic implementations are provided by + /// foreign code. Not all unsafe code actually relies on the purity of + /// `as_ref()`. + #[clippy::version = "1.98.0"] + pub TRUSTED_ASREF, + suspicious, + "unsafe block calls `.as_ref()` multiple times on the same generic receiver" +} + +declare_lint_pass!(TrustedAsref => [TRUSTED_ASREF]); + +impl<'tcx> LateLintPass<'tcx> for TrustedAsref { + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'tcx>) { + if block.rules == hir::BlockCheckMode::UnsafeBlock(hir::UnsafeSource::UserProvided) + && !block.span.in_external_macro(cx.tcx.sess.source_map()) + { + let mut visitor = ForeignAsRefCallVisitor { + cx, + receiver_buckets: FxIndexMap::default(), + }; + visitor.visit_block(block); + + for bucket in visitor.receiver_buckets.into_values() { + for calls in bucket { + check_calls(cx, calls); + } + } + } + } +} + +fn check_calls(cx: &LateContext<'_>, calls: Calls<'_>) { + if calls.spans.len() < 2 { + return; + } + + span_lint_and_help( + cx, + TRUSTED_ASREF, + calls.spans, + "unsafe block calls `as_ref` multiple times on the same receiver", + None, + "consider calling `as_ref()` once and reusing the result to avoid potential unsoundness", + ); +} + +struct Calls<'tcx> { + receiver: &'tcx hir::Expr<'tcx>, + spans: Vec, +} + +struct ForeignAsRefCallVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + receiver_buckets: FxIndexMap>>, +} + +impl<'tcx> Visitor<'tcx> for ForeignAsRefCallVisitor<'_, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { + if matches!( + expr.kind, + hir::ExprKind::Block( + hir::Block { + rules: hir::BlockCheckMode::UnsafeBlock(_), + .. + }, + _ + ) + ) { + return; // Do not descend into nested unsafe block + } + + if let hir::ExprKind::MethodCall(_, receiver, _, _) = expr.kind + && let Some(def_id) = self.cx.typeck_results().type_dependent_def_id(expr.hir_id) + && def_id.opt_parent(self.cx).is_diag_item(self.cx, sym::AsRef) + && let ty::Param(_) = self.cx.typeck_results().expr_ty(receiver).peel_refs().kind() + { + let receiver_bucket = self.receiver_buckets.entry(hash_expr(self.cx, receiver)).or_default(); + + let calls = match receiver_bucket + .iter_mut() + .find(|calls| SpanlessEq::new(self.cx).eq_expr(receiver.span.ctxt(), calls.receiver, receiver)) + { + Some(calls) => calls, + None => receiver_bucket.push_mut(Calls { + receiver, + spans: Vec::new(), + }), + }; + + calls.spans.push(expr.span); + } + + walk_expr(self, expr); + } +} diff --git a/tests/ui/trusted_asref.rs b/tests/ui/trusted_asref.rs new file mode 100644 index 000000000000..cc7a9cc20628 --- /dev/null +++ b/tests/ui/trusted_asref.rs @@ -0,0 +1,113 @@ +#![warn(clippy::trusted_asref)] + +unsafe fn use_slice(_s: *const T, _len: usize) {} + +unsafe fn use_ptr(_s: *const T) {} + +fn bad_generic>(s: T) { + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + //~^ trusted_asref + } +} + +fn bad_impl(s: impl AsRef<[u8]>) { + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + //~^ trusted_asref + } +} + +fn bad_where(s: T) +where + T: AsRef<[u8]>, +{ + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + //~^ trusted_asref + } +} + +fn bad_nested_unsafe>(s: T) { + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + //~^ trusted_asref + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + //~^ trusted_asref + } + } +} + +fn bad_multiple_expr>(s: T) { + unsafe { + let p = s.as_ref().as_ptr(); + //~^ trusted_asref + let len = s.as_ref().len(); + use_slice(p, len); + } +} + +fn bad_multiple_calls>(s: T) { + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + //~^ trusted_asref + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + } +} + +fn good_generic>(s: T) { + let s = s.as_ref(); + unsafe { + use_slice(s.as_ptr(), s.len()); + } +} + +fn good_impl(s: impl AsRef<[u8]>) { + let s = s.as_ref(); + unsafe { + use_slice(s.as_ptr(), s.len()); + } +} + +fn good_where(s: T) +where + T: AsRef<[u8]>, +{ + let s = s.as_ref(); + unsafe { + use_slice(s.as_ptr(), s.len()); + } +} + +fn good_trustworthy_std(s: std::rc::Rc) { + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + } +} + +struct TrustworthyStr(String); + +impl AsRef for TrustworthyStr { + fn as_ref(&self) -> &str { + &self.0 + } +} + +fn good_trustworthy_crate_local(s: TrustworthyStr) { + unsafe { + use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + } +} + +fn good_unrelated_calls(a: impl AsRef, b: impl AsRef) { + unsafe { + use_ptr(a.as_ref() as *const u32); + use_ptr(b.as_ref() as *const u32); + } +} + +fn good_no_unsafe(s: impl AsRef) { + let _ptr = s.as_ref().as_ptr(); + let _len = s.as_ref().len(); +} diff --git a/tests/ui/trusted_asref.stderr b/tests/ui/trusted_asref.stderr new file mode 100644 index 000000000000..4aceed434b55 --- /dev/null +++ b/tests/ui/trusted_asref.stderr @@ -0,0 +1,66 @@ +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:9:19 + | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + = note: `-D clippy::trusted-asref` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::trusted_asref)]` + +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:16:19 + | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:26:19 + | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:33:19 + | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:36:23 + | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:44:17 + | +LL | let p = s.as_ref().as_ptr(); + | ^^^^^^^^^^ +LL | +LL | let len = s.as_ref().len(); + | ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + +error: unsafe block calls `as_ref` multiple times on the same receiver + --> tests/ui/trusted_asref.rs:53:19 + | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ +LL | +LL | use_slice(s.as_ref().as_ptr(), s.as_ref().len()); + | ^^^^^^^^^^ ^^^^^^^^^^ + | + = help: consider calling `as_ref()` once and reusing the result to avoid potential unsoundness + +error: aborting due to 7 previous errors +