Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve invalid_reference_casting lint #112431

Merged
merged 5 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,11 @@ lint_invalid_nan_comparisons_eq_ne = incorrect NaN comparison, NaN cannot be dir

lint_invalid_nan_comparisons_lt_le_gt_ge = incorrect NaN comparison, NaN is not orderable

lint_invalid_reference_casting = casting `&T` to `&mut T` is undefined behavior, even if the reference is unused, consider instead using an `UnsafeCell`
lint_invalid_reference_casting_assign_to_ref = assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
.label = casting happend here

lint_invalid_reference_casting_borrow_as_mut = casting `&T` to `&mut T` is undefined behavior, even if the reference is unused, consider instead using an `UnsafeCell`
.label = casting happend here

lint_lintpass_by_hand = implementing `LintPass` by hand
.help = try using `declare_lint_pass!` or `impl_lint_pass!` instead
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ late_lint_methods!(
BoxPointers: BoxPointers,
PathStatements: PathStatements,
LetUnderscore: LetUnderscore,
InvalidReferenceCasting: InvalidReferenceCasting,
InvalidReferenceCasting: InvalidReferenceCasting::default(),
// Depends on referenced function signatures in expressions
UnusedResults: UnusedResults,
NonUpperCaseGlobals: NonUpperCaseGlobals,
Expand Down
14 changes: 12 additions & 2 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -745,8 +745,18 @@ pub enum InvalidFromUtf8Diag {

// reference_casting.rs
#[derive(LintDiagnostic)]
#[diag(lint_invalid_reference_casting)]
pub struct InvalidReferenceCastingDiag;
pub enum InvalidReferenceCastingDiag {
#[diag(lint_invalid_reference_casting_borrow_as_mut)]
BorrowAsMut {
#[label]
orig_cast: Option<Span>,
},
#[diag(lint_invalid_reference_casting_assign_to_ref)]
AssignToRef {
#[label]
orig_cast: Option<Span>,
},
}

// hidden_unicode_codepoints.rs
#[derive(LintDiagnostic)]
Expand Down
113 changes: 85 additions & 28 deletions compiler/rustc_lint/src/reference_casting.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use rustc_ast::Mutability;
use rustc_hir::{Expr, ExprKind, MutTy, TyKind, UnOp};
use rustc_middle::ty;
use rustc_span::sym;
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, QPath, StmtKind, UnOp};
use rustc_middle::ty::{self, TypeAndMut};
use rustc_span::{sym, Span};

use crate::{lints::InvalidReferenceCastingDiag, LateContext, LateLintPass, LintContext};

Expand All @@ -12,7 +13,6 @@ declare_lint! {
/// ### Example
///
/// ```rust,compile_fail
/// # #![deny(invalid_reference_casting)]
/// fn x(r: &i32) {
/// unsafe {
/// *(r as *const i32 as *mut i32) += 1;
Expand All @@ -30,46 +30,103 @@ declare_lint! {
/// `UnsafeCell` is the only way to obtain aliasable data that is considered
/// mutable.
INVALID_REFERENCE_CASTING,
Allow,
Deny,
"casts of `&T` to `&mut T` without interior mutability"
}

declare_lint_pass!(InvalidReferenceCasting => [INVALID_REFERENCE_CASTING]);
#[derive(Default)]
pub struct InvalidReferenceCasting {
casted: FxHashMap<HirId, Span>,
}

impl_lint_pass!(InvalidReferenceCasting => [INVALID_REFERENCE_CASTING]);

impl<'tcx> LateLintPass<'tcx> for InvalidReferenceCasting {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
let ExprKind::Unary(UnOp::Deref, e) = &expr.kind else {
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx rustc_hir::Stmt<'tcx>) {
let StmtKind::Local(local) = stmt.kind else {
return;
};
let Local { init: Some(init), els: None, .. } = local else {
return;
};

let e = e.peel_blocks();
let e = if let ExprKind::Cast(e, t) = e.kind
&& let TyKind::Ptr(MutTy { mutbl: Mutability::Mut, .. }) = t.kind {
e
} else if let ExprKind::MethodCall(_, expr, [], _) = e.kind
&& let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
&& cx.tcx.is_diagnostic_item(sym::ptr_cast_mut, def_id) {
if is_cast_from_const_to_mut(cx, init) {
self.casted.insert(local.pat.hir_id, init.span);
}
}

fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
// &mut <expr>
let inner = if let ExprKind::AddrOf(_, Mutability::Mut, expr) = expr.kind {
expr
// <expr> = ...
} else if let ExprKind::Assign(expr, _, _) = expr.kind {
expr
// <expr> += ...
} else if let ExprKind::AssignOp(_, expr, _) = expr.kind {
expr
} else {
return;
};

let e = e.peel_blocks();
let e = if let ExprKind::Cast(e, t) = e.kind
&& let TyKind::Ptr(MutTy { mutbl: Mutability::Not, .. }) = t.kind {
e
} else if let ExprKind::Call(path, [arg]) = e.kind
&& let ExprKind::Path(ref qpath) = path.kind
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
&& cx.tcx.is_diagnostic_item(sym::ptr_from_ref, def_id) {
arg
let ExprKind::Unary(UnOp::Deref, e) = &inner.kind else {
return;
};

let orig_cast = if is_cast_from_const_to_mut(cx, e) {
None
} else if let ExprKind::Path(QPath::Resolved(_, path)) = e.kind
&& let Res::Local(hir_id) = &path.res
&& let Some(orig_cast) = self.casted.get(hir_id) {
Some(*orig_cast)
} else {
return;
};

let e = e.peel_blocks();
if let ty::Ref(..) = cx.typeck_results().node_type(e.hir_id).kind() {
cx.emit_spanned_lint(INVALID_REFERENCE_CASTING, expr.span, InvalidReferenceCastingDiag);
}
cx.emit_spanned_lint(
INVALID_REFERENCE_CASTING,
expr.span,
if matches!(expr.kind, ExprKind::AddrOf(..)) {
InvalidReferenceCastingDiag::BorrowAsMut { orig_cast }
} else {
InvalidReferenceCastingDiag::AssignToRef { orig_cast }
},
);
}
}

fn is_cast_from_const_to_mut<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> bool {
let e = e.peel_blocks();

// <expr> as *mut ...
let e = if let ExprKind::Cast(e, t) = e.kind
&& let ty::RawPtr(TypeAndMut { mutbl: Mutability::Mut, .. }) = cx.typeck_results().node_type(t.hir_id).kind() {
e
// <expr>.cast_mut()
} else if let ExprKind::MethodCall(_, expr, [], _) = e.kind
&& let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
&& cx.tcx.is_diagnostic_item(sym::ptr_cast_mut, def_id) {
expr
} else {
return false;
};

let e = e.peel_blocks();

// <expr> as *const ...
let e = if let ExprKind::Cast(e, t) = e.kind
&& let ty::RawPtr(TypeAndMut { mutbl: Mutability::Not, .. }) = cx.typeck_results().node_type(t.hir_id).kind() {
e
// ptr::from_ref(<expr>)
} else if let ExprKind::Call(path, [arg]) = e.kind
&& let ExprKind::Path(ref qpath) = path.kind
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
&& cx.tcx.is_diagnostic_item(sym::ptr_from_ref, def_id) {
arg
} else {
return false;
};

let e = e.peel_blocks();
matches!(cx.typeck_results().node_type(e.hir_id).kind(), ty::Ref(_, _, Mutability::Not))
}
3 changes: 2 additions & 1 deletion library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1893,7 +1893,8 @@ impl<T: ?Sized + fmt::Display> fmt::Display for RefMut<'_, T> {
/// on an _exclusive_ `UnsafeCell<T>`. Even though `T` and `UnsafeCell<T>` have the
/// same memory layout, the following is not allowed and undefined behavior:
///
/// ```rust,no_run
#[cfg_attr(bootstrap, doc = "```rust,no_run")]
#[cfg_attr(not(bootstrap), doc = "```rust,compile_fail")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting case. It's a UB example but it's also something that's fine in both SB and TB as they don't care about get at all, so it seems likely to be fine in the future. Avoiding to lint here is quite easy by just checking whether the pointee has interior mutability with !pointee.is_freeze(), but I'm afraid that this would cause the lint to miss more subtle cases where the type has interior mutability somewhere deeply nested.
@RalfJung what's your opinion on this here? Should the compiler emit a deny-by-default lint on this code below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am totally fine with linting against this, calling get is a lot more clear and the docs for UnsafeCell explicitly say this is UB. It might be "just" library UB instead of language UB, but that's still good enough to justify a lint IMO. We can always revisit this once we have an official aliasing model.

/// # use std::cell::UnsafeCell;
/// unsafe fn not_allowed<T>(ptr: &UnsafeCell<T>) -> &mut T {
/// let t = ptr as *const UnsafeCell<T> as *mut T;
Expand Down
2 changes: 2 additions & 0 deletions src/tools/miri/tests/fail/both_borrows/illegal_write1.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//@revisions: stack tree
//@[tree]compile-flags: -Zmiri-tree-borrows

#![allow(invalid_reference_casting)]

fn main() {
let target = Box::new(42); // has an implicit raw
let xref = &*target;
Expand Down
2 changes: 2 additions & 0 deletions src/tools/miri/tests/fail/stacked_borrows/illegal_write3.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(invalid_reference_casting)]

fn main() {
let target = 42;
// Make sure raw ptr with raw tag cannot mutate frozen location without breaking the shared ref.
Expand Down
1 change: 1 addition & 0 deletions tests/ui/const-generics/issues/issue-100313.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ impl <const B: &'static bool> T<B> {
unsafe {
*(B as *const bool as *mut bool) = false;
//~^ ERROR evaluation of constant value failed [E0080]
//~| ERROR assigning to `&T` is undefined behavior
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions tests/ui/const-generics/issues/issue-100313.stderr
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/issue-100313.rs:10:13
|
LL | *(B as *const bool as *mut bool) = false;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[deny(invalid_reference_casting)]` on by default

error[E0080]: evaluation of constant value failed
--> $DIR/issue-100313.rs:10:13
|
Expand All @@ -10,11 +18,11 @@ note: inside `T::<&true>::set_false`
LL | *(B as *const bool as *mut bool) = false;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `_`
--> $DIR/issue-100313.rs:18:5
--> $DIR/issue-100313.rs:19:5
|
LL | x.set_false();
| ^^^^^^^^^^^^^

error: aborting due to previous error
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
94 changes: 57 additions & 37 deletions tests/ui/lint/reference_casting.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// check-fail

#![feature(ptr_from_ref)]
#![deny(invalid_reference_casting)]

extern "C" {
// N.B., mutability can be easily incorrect in FFI calls -- as
Expand All @@ -10,42 +9,63 @@ extern "C" {
fn int_ffi(c: *mut i32);
}

fn main() {
unsafe fn ref_to_mut() {
let num = &3i32;

let _num = &mut *(num as *const i32 as *mut i32);
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *(num as *const i32).cast_mut();
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *std::ptr::from_ref(num).cast_mut();
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *std::ptr::from_ref({ num }).cast_mut();
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *{ std::ptr::from_ref(num) }.cast_mut();
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *(std::ptr::from_ref({ num }) as *mut i32);
//~^ ERROR casting `&T` to `&mut T` is undefined behavior

let deferred = num as *const i32 as *mut i32;
let _num = &mut *deferred;
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
}

unsafe fn assign_to_ref() {
let s = String::from("Hello");
let a = &s;
unsafe {
let num = &3i32;
let mut_num = &mut 3i32;

(*(a as *const _ as *mut String)).push_str(" world");
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
*(a as *const _ as *mut _) = String::from("Replaced");
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
*(a as *const _ as *mut String) += " world";
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *(num as *const i32 as *mut i32);
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = &mut *(num as *const i32).cast_mut();
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
let _num = *{ num as *const i32 }.cast_mut();
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
*std::ptr::from_ref(num).cast_mut() += 1;
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
*std::ptr::from_ref({ num }).cast_mut() += 1;
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
*{ std::ptr::from_ref(num) }.cast_mut() += 1;
//~^ ERROR casting `&T` to `&mut T` is undefined behavior
*(std::ptr::from_ref({ num }) as *mut i32) += 1;
//~^ ERROR casting `&T` to `&mut T` is undefined behavior

// Shouldn't be warned against
println!("{}", *(num as *const _ as *const i16));
println!("{}", *(mut_num as *mut _ as *mut i16));
ffi(a.as_ptr() as *mut _);
int_ffi(num as *const _ as *mut _);
int_ffi(&3 as *const _ as *mut _);
let mut value = 3;
let value: *const i32 = &mut value;
*(value as *const i16 as *mut i16) = 42;
}
let num = &3i32;

*(a as *const _ as *mut _) = String::from("Replaced");
//~^ ERROR assigning to `&T` is undefined behavior
*(a as *const _ as *mut String) += " world";
//~^ ERROR assigning to `&T` is undefined behavior
*std::ptr::from_ref(num).cast_mut() += 1;
//~^ ERROR assigning to `&T` is undefined behavior
*std::ptr::from_ref({ num }).cast_mut() += 1;
//~^ ERROR assigning to `&T` is undefined behavior
*{ std::ptr::from_ref(num) }.cast_mut() += 1;
//~^ ERROR assigning to `&T` is undefined behavior
*(std::ptr::from_ref({ num }) as *mut i32) += 1;
//~^ ERROR assigning to `&T` is undefined behavior
let value = num as *const i32 as *mut i32;
*value = 1;
//~^ ERROR assigning to `&T` is undefined behavior
}

unsafe fn no_warn() {
let num = &3i32;
let mut_num = &mut 3i32;
let a = &String::from("ffi");

*(num as *const i32 as *mut i32);
println!("{}", *(num as *const _ as *const i16));
println!("{}", *(mut_num as *mut _ as *mut i16));
ffi(a.as_ptr() as *mut _);
int_ffi(num as *const _ as *mut _);
int_ffi(&3 as *const _ as *mut _);
let mut value = 3;
let value: *const i32 = &mut value;
*(value as *const i16 as *mut i16) = 42;
}

fn main() {}
Loading