From 8b5c1d982df05c1f6ce3308459b05fbe32a816d3 Mon Sep 17 00:00:00 2001 From: Urgau Date: Sat, 10 Feb 2024 17:55:17 +0100 Subject: [PATCH] Implement lint against dangerous implicit autorefs --- compiler/rustc_lint/messages.ftl | 4 + compiler/rustc_lint/src/autorefs.rs | 175 +++++++++++++++++++++++++ compiler/rustc_lint/src/lib.rs | 3 + compiler/rustc_lint/src/lints.rs | 20 +++ library/alloc/src/vec/mod.rs | 2 +- tests/ui/lint/implicit_autorefs.fixed | 66 ++++++++++ tests/ui/lint/implicit_autorefs.rs | 66 ++++++++++ tests/ui/lint/implicit_autorefs.stderr | 111 ++++++++++++++++ 8 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 compiler/rustc_lint/src/autorefs.rs create mode 100644 tests/ui/lint/implicit_autorefs.fixed create mode 100644 tests/ui/lint/implicit_autorefs.rs create mode 100644 tests/ui/lint/implicit_autorefs.stderr diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index d379959487191..c06d1f78b060f 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -351,6 +351,10 @@ lint_impl_trait_overcaptures = `{$self_ty}` will capture more lifetimes than pos lint_impl_trait_redundant_captures = all possible in-scope parameters are already captured, so `use<...>` syntax is redundant .suggestion = remove the `use<...>` syntax +lint_implicit_unsafe_autorefs = implicit auto-ref creates a reference to a dereference of a raw pointer + .note = creating a reference requires the pointer to be valid and imposes aliasing requirements + .suggestion = try using a raw pointer method instead; or if this reference is intentional, make it explicit + lint_improper_ctypes = `extern` {$desc} uses type `{$ty}`, which is not FFI-safe .label = not FFI-safe .note = the type is defined here diff --git a/compiler/rustc_lint/src/autorefs.rs b/compiler/rustc_lint/src/autorefs.rs new file mode 100644 index 0000000000000..c98d9817326d5 --- /dev/null +++ b/compiler/rustc_lint/src/autorefs.rs @@ -0,0 +1,175 @@ +use rustc_ast::{BorrowKind, UnOp}; +use rustc_hir::{Expr, ExprKind, Mutability}; +use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, OverloadedDeref}; +use rustc_middle::ty::{TyCtxt, TypeckResults}; +use rustc_session::{declare_lint, declare_lint_pass}; +use rustc_span::sym; + +use crate::lints::{ImplicitUnsafeAutorefsDiag, ImplicitUnsafeAutorefsSuggestion}; +use crate::{LateContext, LateLintPass, LintContext}; + +declare_lint! { + /// The `dangerous_implicit_autorefs` lint checks for implicitly taken references + /// to dereferences of raw pointers. + /// + /// ### Example + /// + /// ```rust + /// use std::ptr::addr_of_mut; + /// + /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] { + /// addr_of_mut!((*ptr)[..16]) + /// // ^^^^^^ this calls `IndexMut::index_mut(&mut ..., ..16)`, + /// // implicitly creating a reference + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// When working with raw pointers it's usually undesirable to create references, + /// since they inflict a lot of safety requirement. Unfortunately, it's possible + /// to take a reference to a dereference of a raw pointer implicitly, which inflicts + /// the usual reference requirements without you even knowing that. + /// + /// If you are sure, you can soundly take a reference, then you can take it explicitly: + /// ```rust + /// # use std::ptr::addr_of_mut; + /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] { + /// addr_of_mut!((&mut *ptr)[..16]) + /// } + /// ``` + /// + /// Otherwise try to find an alternative way to achive your goals that work only with + /// raw pointers: + /// ```rust + /// #![feature(slice_ptr_get)] + /// + /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] { + /// ptr.get_unchecked_mut(..16) + /// } + /// ``` + pub DANGEROUS_IMPLICIT_AUTOREFS, + Warn, + "implicit reference to a dereference of a raw pointer", + report_in_external_macro +} + +declare_lint_pass!(ImplicitAutorefs => [DANGEROUS_IMPLICIT_AUTOREFS]); + +impl<'tcx> LateLintPass<'tcx> for ImplicitAutorefs { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + // This logic has mostly been taken from + // https://github.com/rust-lang/rust/pull/103735#issuecomment-1370420305 + + // 4. Either of the following: + // a. A deref followed by any non-deref place projection (that intermediate + // deref will typically be auto-inserted) + // b. A method call annotated with `#[rustc_no_implicit_refs]`. + // c. A deref followed by a `addr_of!` or `addr_of_mut!`. + let mut is_coming_from_deref = false; + let inner = match expr.kind { + ExprKind::AddrOf(BorrowKind::Raw, _, inner) => match inner.kind { + ExprKind::Unary(UnOp::Deref, inner) => { + is_coming_from_deref = true; + inner + } + _ => return, + }, + ExprKind::Index(base, _idx, _) => base, + ExprKind::MethodCall(_, inner, _, _) + if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && cx.tcx.has_attr(def_id, sym::rustc_no_implicit_autorefs) => + { + inner + } + ExprKind::Call(path, [expr, ..]) + if let ExprKind::Path(ref qpath) = path.kind + && let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id() + && cx.tcx.has_attr(def_id, sym::rustc_no_implicit_autorefs) => + { + expr + } + ExprKind::Field(inner, _) => { + let typeck = cx.typeck_results(); + let adjustments_table = typeck.adjustments(); + if let Some(adjustments) = adjustments_table.get(inner.hir_id) + && let [adjustment] = &**adjustments + && let &Adjust::Deref(Some(OverloadedDeref { .. })) = &adjustment.kind + { + inner + } else { + return; + } + } + _ => return, + }; + + let typeck = cx.typeck_results(); + let adjustments_table = typeck.adjustments(); + + if let Some(adjustments) = adjustments_table.get(inner.hir_id) + && let [adjustment] = &**adjustments + // 3. An automatically inserted reference. + && let Some((mutbl, _implicit_borrow)) = has_implicit_borrow(adjustment) + && let ExprKind::Unary(UnOp::Deref, dereferenced) = + // 2. Any number of place projections + peel_place_mappers(cx.tcx, typeck, inner).kind + // 1. Deref of a raw pointer + && typeck.expr_ty(dereferenced).is_unsafe_ptr() + { + cx.emit_span_lint( + DANGEROUS_IMPLICIT_AUTOREFS, + expr.span.source_callsite(), + ImplicitUnsafeAutorefsDiag { + suggestion: ImplicitUnsafeAutorefsSuggestion { + mutbl: mutbl.ref_prefix_str(), + deref: if is_coming_from_deref { "*" } else { "" }, + start_span: inner.span.shrink_to_lo(), + end_span: inner.span.shrink_to_hi(), + }, + }, + ) + } + } +} + +/// Peels expressions from `expr` that can map a place. +fn peel_place_mappers<'tcx>( + _tcx: TyCtxt<'tcx>, + _typeck: &TypeckResults<'tcx>, + mut expr: &'tcx Expr<'tcx>, +) -> &'tcx Expr<'tcx> { + loop { + match expr.kind { + ExprKind::Index(base, _idx, _) => { + expr = &base; + } + ExprKind::Field(e, _) => expr = &e, + _ => break expr, + } + } +} + +enum ImplicitBorrowKind { + Deref, + Borrow, +} + +/// Test if some adjustment has some implicit borrow +/// +/// Returns `Some(mutability)` if the argument adjustment has implicit borrow in it. +fn has_implicit_borrow( + Adjustment { kind, .. }: &Adjustment<'_>, +) -> Option<(Mutability, ImplicitBorrowKind)> { + match kind { + &Adjust::Deref(Some(OverloadedDeref { mutbl, .. })) => { + Some((mutbl, ImplicitBorrowKind::Deref)) + } + &Adjust::Borrow(AutoBorrow::Ref(_, mutbl)) => { + Some((mutbl.into(), ImplicitBorrowKind::Borrow)) + } + _ => None, + } +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 81352af3d48fa..fab6390c75e74 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -44,6 +44,7 @@ mod async_closures; mod async_fn_in_trait; +mod autorefs; pub mod builtin; mod context; mod deref_into_dyn_supertrait; @@ -90,6 +91,7 @@ mod unused; use async_closures::AsyncClosureUsage; use async_fn_in_trait::AsyncFnInTrait; +use autorefs::*; use builtin::*; use deref_into_dyn_supertrait::*; use drop_forget_useless::*; @@ -207,6 +209,7 @@ late_lint_methods!( PathStatements: PathStatements, LetUnderscore: LetUnderscore, InvalidReferenceCasting: InvalidReferenceCasting, + ImplicitAutorefs: ImplicitAutorefs, // Depends on referenced function signatures in expressions UnusedResults: UnusedResults, UnitBindings: UnitBindings, diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index a861894a44493..111bcf43a6874 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/compiler/rustc_lint/src/lints.rs @@ -54,6 +54,26 @@ pub(crate) enum ShadowedIntoIterDiagSub { }, } +// autorefs.rs +#[derive(LintDiagnostic)] +#[diag(lint_implicit_unsafe_autorefs)] +#[note] +pub(crate) struct ImplicitUnsafeAutorefsDiag { + #[subdiagnostic] + pub suggestion: ImplicitUnsafeAutorefsSuggestion, +} + +#[derive(Subdiagnostic)] +#[multipart_suggestion(lint_suggestion, applicability = "maybe-incorrect")] +pub(crate) struct ImplicitUnsafeAutorefsSuggestion { + pub mutbl: &'static str, + pub deref: &'static str, + #[suggestion_part(code = "({mutbl}{deref}")] + pub start_span: Span, + #[suggestion_part(code = ")")] + pub end_span: Span, +} + // builtin.rs #[derive(LintDiagnostic)] #[diag(lint_builtin_while_true)] diff --git a/library/alloc/src/vec/mod.rs b/library/alloc/src/vec/mod.rs index 830512ceee87b..0fc2ce7022c6a 100644 --- a/library/alloc/src/vec/mod.rs +++ b/library/alloc/src/vec/mod.rs @@ -2549,7 +2549,7 @@ impl Vec { #[cfg(not(no_global_oom_handling))] #[inline] unsafe fn append_elements(&mut self, other: *const [T]) { - let count = unsafe { (*other).len() }; + let count = other.len(); self.reserve(count); let len = self.len(); unsafe { ptr::copy_nonoverlapping(other as *const T, self.as_mut_ptr().add(len), count) }; diff --git a/tests/ui/lint/implicit_autorefs.fixed b/tests/ui/lint/implicit_autorefs.fixed new file mode 100644 index 0000000000000..d69f8c8584864 --- /dev/null +++ b/tests/ui/lint/implicit_autorefs.fixed @@ -0,0 +1,66 @@ +//@ check-pass +//@ run-rustfix + +#![allow(dead_code)] // for the rustfix-ed code + +use std::mem::ManuallyDrop; +use std::ptr::addr_of_mut; +use std::ptr::addr_of; +use std::ops::Deref; + +unsafe fn test_const(ptr: *const [u8]) -> *const [u8] { + addr_of!((&(*ptr))[..16]) + //~^ WARN implicit auto-ref +} + +struct Test { + field: [u8], +} + +unsafe fn test_field(ptr: *const Test) -> *const [u8] { + let l = (&(*ptr).field).len(); + //~^ WARN implicit auto-ref + + addr_of!((&(*ptr).field)[..l - 1]) + //~^ WARN implicit auto-ref +} + +unsafe fn test_builtin_index(a: *mut [String]) { + _ = (&(*a)[0]).len(); + //~^ WARN implicit auto-ref + + _ = (&(&(*a))[..1][0]).len(); + //~^ WARN implicit auto-ref + //~^^ WARN implicit auto-ref +} + +unsafe fn test_overloaded_deref_const(ptr: *const ManuallyDrop) { + _ = addr_of!((&(*ptr)).field); + //~^ WARN implicit auto-ref +} + +unsafe fn test_overloaded_deref_mut(ptr: *mut ManuallyDrop) { + _ = addr_of_mut!((&mut (*ptr)).field); + //~^ WARN implicit auto-ref +} + +unsafe fn test_manually_overloaded_deref() { + struct W(T); + + impl Deref for W { + type Target = T; + fn deref(&self) -> &T { &self.0 } + } + + let w: W = W(5); + let w = addr_of!(w); + let _p: *const i32 = addr_of!(*(&**w)); + //~^ WARN implicit auto-ref +} + +unsafe fn test_no_attr(ptr: *mut ManuallyDrop) { + ptr.write(ManuallyDrop::new(1)); // should not warn, as `ManuallyDrop::write` is not + // annotated with `#[rustc_no_implicit_auto_ref]` +} + +fn main() {} diff --git a/tests/ui/lint/implicit_autorefs.rs b/tests/ui/lint/implicit_autorefs.rs new file mode 100644 index 0000000000000..16b035a01a5c6 --- /dev/null +++ b/tests/ui/lint/implicit_autorefs.rs @@ -0,0 +1,66 @@ +//@ check-pass +//@ run-rustfix + +#![allow(dead_code)] // for the rustfix-ed code + +use std::mem::ManuallyDrop; +use std::ptr::addr_of_mut; +use std::ptr::addr_of; +use std::ops::Deref; + +unsafe fn test_const(ptr: *const [u8]) -> *const [u8] { + addr_of!((*ptr)[..16]) + //~^ WARN implicit auto-ref +} + +struct Test { + field: [u8], +} + +unsafe fn test_field(ptr: *const Test) -> *const [u8] { + let l = (*ptr).field.len(); + //~^ WARN implicit auto-ref + + addr_of!((*ptr).field[..l - 1]) + //~^ WARN implicit auto-ref +} + +unsafe fn test_builtin_index(a: *mut [String]) { + _ = (*a)[0].len(); + //~^ WARN implicit auto-ref + + _ = (*a)[..1][0].len(); + //~^ WARN implicit auto-ref + //~^^ WARN implicit auto-ref +} + +unsafe fn test_overloaded_deref_const(ptr: *const ManuallyDrop) { + _ = addr_of!((*ptr).field); + //~^ WARN implicit auto-ref +} + +unsafe fn test_overloaded_deref_mut(ptr: *mut ManuallyDrop) { + _ = addr_of_mut!((*ptr).field); + //~^ WARN implicit auto-ref +} + +unsafe fn test_manually_overloaded_deref() { + struct W(T); + + impl Deref for W { + type Target = T; + fn deref(&self) -> &T { &self.0 } + } + + let w: W = W(5); + let w = addr_of!(w); + let _p: *const i32 = addr_of!(**w); + //~^ WARN implicit auto-ref +} + +unsafe fn test_no_attr(ptr: *mut ManuallyDrop) { + ptr.write(ManuallyDrop::new(1)); // should not warn, as `ManuallyDrop::write` is not + // annotated with `#[rustc_no_implicit_auto_ref]` +} + +fn main() {} diff --git a/tests/ui/lint/implicit_autorefs.stderr b/tests/ui/lint/implicit_autorefs.stderr new file mode 100644 index 0000000000000..b6e25f4f57656 --- /dev/null +++ b/tests/ui/lint/implicit_autorefs.stderr @@ -0,0 +1,111 @@ +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:12:14 + | +LL | addr_of!((*ptr)[..16]) + | ^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements + = note: `#[warn(dangerous_implicit_autorefs)]` on by default +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | addr_of!((&(*ptr))[..16]) + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:21:13 + | +LL | let l = (*ptr).field.len(); + | ^^^^^^^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | let l = (&(*ptr).field).len(); + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:24:14 + | +LL | addr_of!((*ptr).field[..l - 1]) + | ^^^^^^^^^^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | addr_of!((&(*ptr).field)[..l - 1]) + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:29:9 + | +LL | _ = (*a)[0].len(); + | ^^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | _ = (&(*a)[0]).len(); + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:32:9 + | +LL | _ = (*a)[..1][0].len(); + | ^^^^^^^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | _ = (&(*a)[..1][0]).len(); + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:32:9 + | +LL | _ = (*a)[..1][0].len(); + | ^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | _ = (&(*a))[..1][0].len(); + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:38:18 + | +LL | _ = addr_of!((*ptr).field); + | ^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | _ = addr_of!((&(*ptr)).field); + | ++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:43:22 + | +LL | _ = addr_of_mut!((*ptr).field); + | ^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | _ = addr_of_mut!((&mut (*ptr)).field); + | +++++ + + +warning: implicit auto-ref creates a reference to a dereference of a raw pointer + --> $DIR/implicit_autorefs.rs:57:26 + | +LL | let _p: *const i32 = addr_of!(**w); + | ^^^^^^^^^^^^^ + | + = note: creating a reference requires the pointer to be valid and imposes aliasing requirements +help: try using a raw pointer method instead; or if this reference is intentional, make it explicit + | +LL | let _p: *const i32 = addr_of!(*(&**w)); + | +++ + + +warning: 9 warnings emitted +