-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Llint for casting between raw slice pointers with different element sizes #8445
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
use clippy_utils::{diagnostics::span_lint_and_then, meets_msrv, msrvs, source::snippet_opt}; | ||
use if_chain::if_chain; | ||
use rustc_ast::Mutability; | ||
use rustc_hir::{Expr, ExprKind, Node}; | ||
use rustc_lint::LateContext; | ||
use rustc_middle::ty::{self, layout::LayoutOf, Ty, TypeAndMut}; | ||
use rustc_semver::RustcVersion; | ||
|
||
use super::CAST_SLICE_DIFFERENT_SIZES; | ||
|
||
fn is_child_of_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { | ||
let map = cx.tcx.hir(); | ||
if_chain! { | ||
if let Some(parent_id) = map.find_parent_node(expr.hir_id); | ||
if let Some(parent) = map.find(parent_id); | ||
then { | ||
let expr = match parent { | ||
Node::Block(block) => { | ||
if let Some(parent_expr) = block.expr { | ||
parent_expr | ||
} else { | ||
return false; | ||
} | ||
}, | ||
Node::Expr(expr) => expr, | ||
_ => return false, | ||
}; | ||
|
||
matches!(expr.kind, ExprKind::Cast(..)) | ||
} else { | ||
false | ||
} | ||
} | ||
} | ||
|
||
pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, msrv: &Option<RustcVersion>) { | ||
// suggestion is invalid if `ptr::slice_from_raw_parts` does not exist | ||
if !meets_msrv(msrv.as_ref(), &msrvs::PTR_SLICE_RAW_PARTS) { | ||
return; | ||
} | ||
|
||
// if this cast is the child of another cast expression then don't emit something for it, the full | ||
// chain will be analyzed | ||
if is_child_of_cast(cx, expr) { | ||
return; | ||
} | ||
|
||
if let Some((from_slice_ty, to_slice_ty)) = expr_cast_chain_tys(cx, expr) { | ||
if let (Ok(from_layout), Ok(to_layout)) = (cx.layout_of(from_slice_ty.ty), cx.layout_of(to_slice_ty.ty)) { | ||
let from_size = from_layout.size.bytes(); | ||
let to_size = to_layout.size.bytes(); | ||
if from_size != to_size && from_size != 0 && to_size != 0 { | ||
span_lint_and_then( | ||
cx, | ||
CAST_SLICE_DIFFERENT_SIZES, | ||
expr.span, | ||
&format!( | ||
"casting between raw pointers to `[{}]` (element size {}) and `[{}]` (element size {}) does not adjust the count", | ||
from_slice_ty, from_size, to_slice_ty, to_size, | ||
), | ||
|diag| { | ||
let cast_expr = match expr.kind { | ||
ExprKind::Cast(cast_expr, ..) => cast_expr, | ||
_ => unreachable!("expr should be a cast as checked by expr_cast_chain_tys"), | ||
}; | ||
let ptr_snippet = snippet_opt(cx, cast_expr.span).unwrap(); | ||
|
||
let (mutbl_fn_str, mutbl_ptr_str) = match to_slice_ty.mutbl { | ||
Mutability::Mut => ("_mut", "mut"), | ||
Mutability::Not => ("", "const"), | ||
}; | ||
let sugg = format!( | ||
"core::ptr::slice_from_raw_parts{mutbl_fn_str}({ptr_snippet} as *{mutbl_ptr_str} {to_slice_ty}, ..)" | ||
); | ||
|
||
diag.span_suggestion( | ||
expr.span, | ||
&format!("replace with `ptr::slice_from_raw_parts{mutbl_fn_str}`"), | ||
sugg, | ||
rustc_errors::Applicability::HasPlaceholders, | ||
); | ||
}, | ||
); | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// Returns the type T of the pointed to *const [T] or *mut [T] and the mutability of the slice if | ||
/// the type is one of those slices | ||
fn get_raw_slice_ty_mut(ty: Ty<'_>) -> Option<TypeAndMut<'_>> { | ||
match ty.kind() { | ||
ty::RawPtr(TypeAndMut { ty: slice_ty, mutbl }) => match slice_ty.kind() { | ||
ty::Slice(ty) => Some(TypeAndMut { ty: *ty, mutbl: *mutbl }), | ||
_ => None, | ||
}, | ||
_ => None, | ||
} | ||
} | ||
|
||
/// Returns the pair (original ptr T, final ptr U) if the expression is composed of casts | ||
/// Returns None if the expr is not a Cast | ||
fn expr_cast_chain_tys<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option<(TypeAndMut<'tcx>, TypeAndMut<'tcx>)> { | ||
if let ExprKind::Cast(cast_expr, _cast_to_hir_ty) = expr.peel_blocks().kind { | ||
let cast_to = cx.typeck_results().expr_ty(expr); | ||
let to_slice_ty = get_raw_slice_ty_mut(cast_to)?; | ||
if let Some((inner_from_ty, _inner_to_ty)) = expr_cast_chain_tys(cx, cast_expr) { | ||
Some((inner_from_ty, to_slice_ty)) | ||
} else { | ||
let cast_from = cx.typeck_results().expr_ty(cast_expr); | ||
let from_slice_ty = get_raw_slice_ty_mut(cast_from)?; | ||
Some((from_slice_ty, to_slice_ty)) | ||
} | ||
} else { | ||
None | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
fn main() { | ||
let x: [i32; 3] = [1_i32, 2, 3]; | ||
let r_x = &x; | ||
// Check casting through multiple bindings | ||
// Because it's separate, it does not check the cast back to something of the same size | ||
let a = r_x as *const [i32]; | ||
let b = a as *const [u8]; | ||
let c = b as *const [u32]; | ||
|
||
// loses data | ||
let loss = r_x as *const [i32] as *const [u8]; | ||
|
||
// Cast back to same size but different type loses no data, just type conversion | ||
// This is weird code but there's no reason for this lint specifically to fire *twice* on it | ||
let restore = r_x as *const [i32] as *const [u8] as *const [u32]; | ||
|
||
// Check casting through blocks is detected | ||
let loss_block_1 = { r_x as *const [i32] } as *const [u8]; | ||
let loss_block_2 = { | ||
let _ = (); | ||
r_x as *const [i32] | ||
} as *const [u8]; | ||
|
||
// Check that resores of the same size are detected through blocks | ||
let restore_block_1 = { r_x as *const [i32] } as *const [u8] as *const [u32]; | ||
let restore_block_2 = { ({ r_x as *const [i32] }) as *const [u8] } as *const [u32]; | ||
let restore_block_3 = { | ||
let _ = (); | ||
({ | ||
let _ = (); | ||
r_x as *const [i32] | ||
}) as *const [u8] | ||
} as *const [u32]; | ||
|
||
// Check that the result of a long chain of casts is detected | ||
let long_chain_loss = r_x as *const [i32] as *const [u32] as *const [u16] as *const [i8] as *const [u8]; | ||
let long_chain_restore = | ||
r_x as *const [i32] as *const [u32] as *const [u16] as *const [i8] as *const [u8] as *const [u32]; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count | ||
--> $DIR/cast_slice_different_sizes.rs:7:13 | ||
| | ||
LL | let b = a as *const [u8]; | ||
| ^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(a as *const u8, ..)` | ||
| | ||
= note: `#[deny(clippy::cast_slice_different_sizes)]` on by default | ||
|
||
error: casting between raw pointers to `[u8]` (element size 1) and `[u32]` (element size 4) does not adjust the count | ||
--> $DIR/cast_slice_different_sizes.rs:8:13 | ||
| | ||
LL | let c = b as *const [u32]; | ||
| ^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(b as *const u32, ..)` | ||
|
||
error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count | ||
--> $DIR/cast_slice_different_sizes.rs:11:16 | ||
| | ||
LL | let loss = r_x as *const [i32] as *const [u8]; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(r_x as *const [i32] as *const u8, ..)` | ||
|
||
error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count | ||
--> $DIR/cast_slice_different_sizes.rs:18:24 | ||
| | ||
LL | let loss_block_1 = { r_x as *const [i32] } as *const [u8]; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts({ r_x as *const [i32] } as *const u8, ..)` | ||
|
||
error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count | ||
--> $DIR/cast_slice_different_sizes.rs:19:24 | ||
| | ||
LL | let loss_block_2 = { | ||
| ________________________^ | ||
LL | | let _ = (); | ||
LL | | r_x as *const [i32] | ||
LL | | } as *const [u8]; | ||
| |____________________^ | ||
| | ||
help: replace with `ptr::slice_from_raw_parts` | ||
| | ||
LL ~ let loss_block_2 = core::ptr::slice_from_raw_parts({ | ||
LL + let _ = (); | ||
LL + r_x as *const [i32] | ||
LL ~ } as *const u8, ..); | ||
| | ||
|
||
error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count | ||
--> $DIR/cast_slice_different_sizes.rs:36:27 | ||
| | ||
LL | let long_chain_loss = r_x as *const [i32] as *const [u32] as *const [u16] as *const [i8] as *const [u8]; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(r_x as *const [i32] as *const [u32] as *const [u16] as *const [i8] as *const u8, ..)` | ||
|
||
error: aborting due to 6 previous errors | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there could be cases where the code is actually correct, even if surprising.
If that is the case, I worry that having a deny by default lint might lead to a downgrade to nursery. So in this case using the
suspicious
group might be a better choice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exceptions when one or both target slices have a zero sized inner type should cover what I believe are the only correct uses:
&[()]
(because there's zero bytes operated on).*const [()]
that has the correct inner data and size metadata (would be really weird but probably not wrong)I asked on the Rust community discord if anyone had any other ideas about possible correct uses of a cast like this and nobody had any ideas.
Any time that data is actually accessed behind the pointer it either can be done with a thin pointer or with a slice pointer with a correct length. While I suppose it's technically the dereference that could cause behavior, I don't think there's ever a way to correctly use a slice pointer with the incorrect length, so I believe that it should be deny by default.