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.

Sign up for GitHub

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

Some cleanup and improvement for invalid ref casting impl #118909

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Some cleanup and improvement for invalid ref casting impl #118909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07e6224
Extract casting detection logic in it's own function
Urgau Dec 12, 2023
97a2613
Refactor and rename some functions in ref casting lint
Urgau Dec 12, 2023
4839cca
Recurse into let bindings if possible in ref casting lint
Urgau Dec 12, 2023
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
138 changes: 81 additions & 57 deletions compiler/rustc_lint/src/reference_casting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,59 +37,73 @@ declare_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 Some((is_assignment, e)) = is_operation_we_care_about(cx, expr) else {
return;
};

let init = cx.expr_or_init(e);

let Some(ty_has_interior_mutability) = is_cast_from_const_to_mut(cx, init) else {
return;
};
let orig_cast = if init.span != e.span { Some(init.span) } else { None };
let ty_has_interior_mutability = ty_has_interior_mutability.then_some(());

cx.emit_spanned_lint(
INVALID_REFERENCE_CASTING,
expr.span,
if is_assignment {
InvalidReferenceCastingDiag::AssignToRef { orig_cast, ty_has_interior_mutability }
} else {
InvalidReferenceCastingDiag::BorrowAsMut { orig_cast, ty_has_interior_mutability }
},
);
if let Some((e, pat)) = borrow_or_assign(cx, expr) {
est31 marked this conversation as resolved.
Show resolved Hide resolved
if matches!(pat, PatternKind::Borrow { mutbl: Mutability::Mut } | PatternKind::Assign) {
let init = cx.expr_or_init(e);

let Some(ty_has_interior_mutability) = is_cast_from_ref_to_mut_ptr(cx, init) else {
return;
};
let orig_cast = if init.span != e.span { Some(init.span) } else { None };
let ty_has_interior_mutability = ty_has_interior_mutability.then_some(());

cx.emit_spanned_lint(
INVALID_REFERENCE_CASTING,
expr.span,
if pat == PatternKind::Assign {
InvalidReferenceCastingDiag::AssignToRef {
orig_cast,
ty_has_interior_mutability,
}
} else {
InvalidReferenceCastingDiag::BorrowAsMut {
orig_cast,
ty_has_interior_mutability,
}
},
);
}
}
}
}

fn is_operation_we_care_about<'tcx>(
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum PatternKind {
Borrow { mutbl: Mutability },
Assign,
}

fn borrow_or_assign<'tcx>(
cx: &LateContext<'tcx>,
e: &'tcx Expr<'tcx>,
) -> Option<(bool, &'tcx Expr<'tcx>)> {
fn deref_assign_or_addr_of<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<(bool, &'tcx Expr<'tcx>)> {
// &mut <expr>
let inner = if let ExprKind::AddrOf(_, Mutability::Mut, expr) = expr.kind {
expr
) -> Option<(&'tcx Expr<'tcx>, PatternKind)> {
fn deref_assign_or_addr_of<'tcx>(
expr: &'tcx Expr<'tcx>,
) -> Option<(&'tcx Expr<'tcx>, PatternKind)> {
// &(mut) <expr>
let (inner, pat) = if let ExprKind::AddrOf(_, mutbl, expr) = expr.kind {
(expr, PatternKind::Borrow { mutbl })
// <expr> = ...
} else if let ExprKind::Assign(expr, _, _) = expr.kind {
expr
(expr, PatternKind::Assign)
// <expr> += ...
} else if let ExprKind::AssignOp(_, expr, _) = expr.kind {
expr
(expr, PatternKind::Assign)
} else {
return None;
};

if let ExprKind::Unary(UnOp::Deref, e) = &inner.kind {
Some((!matches!(expr.kind, ExprKind::AddrOf(..)), e))
} else {
None
}
// *<inner>
let ExprKind::Unary(UnOp::Deref, e) = &inner.kind else {
return None;
};
Some((e, pat))
}

fn ptr_write<'tcx>(
cx: &LateContext<'tcx>,
e: &'tcx Expr<'tcx>,
) -> Option<(bool, &'tcx Expr<'tcx>)> {
) -> Option<(&'tcx Expr<'tcx>, PatternKind)> {
if let ExprKind::Call(path, [arg_ptr, _arg_val]) = e.kind
&& let ExprKind::Path(ref qpath) = path.kind
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
Expand All @@ -98,7 +112,7 @@ fn is_operation_we_care_about<'tcx>(
Some(sym::ptr_write | sym::ptr_write_volatile | sym::ptr_write_unaligned)
)
{
Some((true, arg_ptr))
Some((arg_ptr, PatternKind::Assign))
} else {
None
}
Expand All @@ -107,20 +121,39 @@ fn is_operation_we_care_about<'tcx>(
deref_assign_or_addr_of(e).or_else(|| ptr_write(cx, e))
}

fn is_cast_from_const_to_mut<'tcx>(
fn is_cast_from_ref_to_mut_ptr<'tcx>(
cx: &LateContext<'tcx>,
orig_expr: &'tcx Expr<'tcx>,
) -> Option<bool> {
let mut need_check_freeze = false;
let mut e = orig_expr;

let end_ty = cx.typeck_results().node_type(orig_expr.hir_id);

// Bail out early if the end type is **not** a mutable pointer.
if !matches!(end_ty.kind(), ty::RawPtr(TypeAndMut { ty: _, mutbl: Mutability::Mut })) {
return None;
}

let (e, need_check_freeze) = peel_casts(cx, orig_expr);

let start_ty = cx.typeck_results().node_type(e.hir_id);
if let ty::Ref(_, inner_ty, Mutability::Not) = start_ty.kind() {
// If an UnsafeCell method is involved, we need to additionally check the
// inner type for the presence of the Freeze trait (ie does NOT contain
// an UnsafeCell), since in that case we would incorrectly lint on valid casts.
//
// Except on the presence of non concrete skeleton types (ie generics)
// since there is no way to make it safe for arbitrary types.
let inner_ty_has_interior_mutability =
!inner_ty.is_freeze(cx.tcx, cx.param_env) && inner_ty.has_concrete_skeleton();
(!need_check_freeze || !inner_ty_has_interior_mutability)
.then_some(inner_ty_has_interior_mutability)
} else {
None
}
}

fn peel_casts<'tcx>(cx: &LateContext<'tcx>, mut e: &'tcx Expr<'tcx>) -> (&'tcx Expr<'tcx>, bool) {
let mut gone_trough_unsafe_cell_raw_get = false;

loop {
e = e.peel_blocks();
// <expr> as ...
Expand All @@ -145,27 +178,18 @@ fn is_cast_from_const_to_mut<'tcx>(
)
{
if cx.tcx.is_diagnostic_item(sym::unsafe_cell_raw_get, def_id) {
need_check_freeze = true;
gone_trough_unsafe_cell_raw_get = true;
}
arg
} else {
break;
let init = cx.expr_or_init(e);
if init.hir_id != e.hir_id {
init
} else {
break;
}
};
}

let start_ty = cx.typeck_results().node_type(e.hir_id);
if let ty::Ref(_, inner_ty, Mutability::Not) = start_ty.kind() {
// If an UnsafeCell method is involved we need to additionally check the
// inner type for the presence of the Freeze trait (ie does NOT contain
// an UnsafeCell), since in that case we would incorrectly lint on valid casts.
//
// We also consider non concrete skeleton types (ie generics)
// to be an issue since there is no way to make it safe for abitrary types.
let inner_ty_has_interior_mutability =
!inner_ty.is_freeze(cx.tcx, cx.param_env) && inner_ty.has_concrete_skeleton();
(!need_check_freeze || !inner_ty_has_interior_mutability)
.then_some(inner_ty_has_interior_mutability)
} else {
None
}
(e, gone_trough_unsafe_cell_raw_get)
}
7 changes: 7 additions & 0 deletions tests/ui/lint/reference_casting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ unsafe fn assign_to_ref() {
*((&std::cell::UnsafeCell::new(0)) as *const _ as *mut i32) = 5;
//~^ 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
let value = num as *const i32;
let value = value as *mut i32;
*value = 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
Expand Down
36 changes: 28 additions & 8 deletions tests/ui/lint/reference_casting.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,27 @@ LL | *value = 1;
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:120:5
--> $DIR/reference_casting.rs:121:5
|
LL | let value = value as *mut i32;
| ----------------- casting happend here
LL | *value = 1;
| ^^^^^^^^^^
|
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:124:5
|
LL | let value = num as *const i32 as *mut i32;
| ----------------------------- casting happend here
LL | *value = 1;
| ^^^^^^^^^^
|
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:127:5
|
LL | let value = num as *const i32 as *mut i32;
| ----------------------------- casting happend here
Expand All @@ -297,23 +317,23 @@ LL | *value_rebind = 1;
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:122:5
--> $DIR/reference_casting.rs:129:5
|
LL | *(num as *const i32).cast::<i32>().cast_mut() = 2;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:124:5
--> $DIR/reference_casting.rs:131:5
|
LL | *(num as *const _ as usize as *mut i32) = 2;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:126:5
--> $DIR/reference_casting.rs:133:5
|
LL | let value = num as *const i32 as *mut i32;
| ----------------------------- casting happend here
Expand All @@ -324,7 +344,7 @@ LL | std::ptr::write(value, 2);
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:128:5
--> $DIR/reference_casting.rs:135:5
|
LL | let value = num as *const i32 as *mut i32;
| ----------------------------- casting happend here
Expand All @@ -335,7 +355,7 @@ LL | std::ptr::write_unaligned(value, 2);
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:130:5
--> $DIR/reference_casting.rs:137:5
|
LL | let value = num as *const i32 as *mut i32;
| ----------------------------- casting happend here
Expand All @@ -346,12 +366,12 @@ LL | std::ptr::write_volatile(value, 2);
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: assigning to `&T` is undefined behavior, consider using an `UnsafeCell`
--> $DIR/reference_casting.rs:134:9
--> $DIR/reference_casting.rs:141:9
|
LL | *(this as *const _ as *mut _) = a;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: for more information, visit <https://doc.rust-lang.org/book/ch15-05-interior-mutability.html>

error: aborting due to 40 previous errors
error: aborting due to 42 previous errors

Loading