Skip to content

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 1 commit into from
Mar 6, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3076,6 +3076,7 @@ Released 2018-09-13
[`cast_ptr_alignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_ptr_alignment
[`cast_ref_to_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_ref_to_mut
[`cast_sign_loss`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_sign_loss
[`cast_slice_different_sizes`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_slice_different_sizes
[`char_lit_as_u8`]: https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8
[`chars_last_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_last_cmp
[`chars_next_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_next_cmp
Expand Down
117 changes: 117 additions & 0 deletions clippy_lints/src/casts/cast_slice_different_sizes.rs
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
}
}
48 changes: 48 additions & 0 deletions clippy_lints/src/casts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ mod cast_precision_loss;
mod cast_ptr_alignment;
mod cast_ref_to_mut;
mod cast_sign_loss;
mod cast_slice_different_sizes;
mod char_lit_as_u8;
mod fn_to_numeric_cast;
mod fn_to_numeric_cast_any;
Expand Down Expand Up @@ -409,6 +410,50 @@ declare_clippy_lint! {
"casts from an enum type to an integral type which will truncate the value"
}

declare_clippy_lint! {
/// Checks for `as` casts between raw pointers to slices with differently sized elements.
///
/// ### Why is this bad?
/// The produced raw pointer to a slice does not update its length metadata. The produced
/// pointer will point to a different number of bytes than the original pointer because the
/// length metadata of a raw slice pointer is in elements rather than bytes.
/// Producing a slice reference from the raw pointer will either create a slice with
/// less data (which can be surprising) or create a slice with more data and cause Undefined Behavior.
///
/// ### Example
/// // Missing data
/// ```rust
/// let a = [1_i32, 2, 3, 4];
/// let p = &a as *const [i32] as *const [u8];
/// unsafe {
/// println!("{:?}", &*p);
/// }
/// ```
/// // Undefined Behavior (note: also potential alignment issues)
/// ```rust
/// let a = [1_u8, 2, 3, 4];
/// let p = &a as *const [u8] as *const [u32];
/// unsafe {
/// println!("{:?}", &*p);
/// }
/// ```
/// Instead use `ptr::slice_from_raw_parts` to construct a slice from a data pointer and the correct length
/// ```rust
/// let a = [1_i32, 2, 3, 4];
/// let old_ptr = &a as *const [i32];
/// // The data pointer is cast to a pointer to the target `u8` not `[u8]`
/// // The length comes from the known length of 4 i32s times the 4 bytes per i32
/// let new_ptr = core::ptr::slice_from_raw_parts(old_ptr as *const u8, 16);
/// unsafe {
/// println!("{:?}", &*new_ptr);
/// }
/// ```
#[clippy::version = "1.60.0"]
pub CAST_SLICE_DIFFERENT_SIZES,
correctness,
Copy link
Contributor

@llogiq llogiq Feb 26, 2022

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.

Copy link
Contributor Author

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:

  • To get the length metadata of a slice pointer on stable Rust by going through a &[()] (because there's zero bytes operated on).
  • To come from a pointer like *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.

"casting using `as` between raw pointers to slices of types with different sizes"
}

pub struct Casts {
msrv: Option<RustcVersion>,
}
Expand All @@ -428,6 +473,7 @@ impl_lint_pass!(Casts => [
CAST_LOSSLESS,
CAST_REF_TO_MUT,
CAST_PTR_ALIGNMENT,
CAST_SLICE_DIFFERENT_SIZES,
UNNECESSARY_CAST,
FN_TO_NUMERIC_CAST_ANY,
FN_TO_NUMERIC_CAST,
Expand Down Expand Up @@ -478,6 +524,8 @@ impl<'tcx> LateLintPass<'tcx> for Casts {
cast_ref_to_mut::check(cx, expr);
cast_ptr_alignment::check(cx, expr);
char_lit_as_u8::check(cx, expr);
ptr_as_ptr::check(cx, expr, &self.msrv);
cast_slice_different_sizes::check(cx, expr, &self.msrv);
}

extract_msrv_attr!(LateContext);
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
LintId::of(booleans::NONMINIMAL_BOOL),
LintId::of(casts::CAST_ENUM_TRUNCATION),
LintId::of(casts::CAST_REF_TO_MUT),
LintId::of(casts::CAST_SLICE_DIFFERENT_SIZES),
LintId::of(casts::CHAR_LIT_AS_U8),
LintId::of(casts::FN_TO_NUMERIC_CAST),
LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_correctness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
LintId::of(booleans::LOGIC_BUG),
LintId::of(casts::CAST_REF_TO_MUT),
LintId::of(casts::CAST_SLICE_DIFFERENT_SIZES),
LintId::of(copies::IFS_SAME_COND),
LintId::of(copies::IF_SAME_THEN_ELSE),
LintId::of(derive::DERIVE_HASH_XOR_EQ),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ store.register_lints(&[
casts::CAST_PTR_ALIGNMENT,
casts::CAST_REF_TO_MUT,
casts::CAST_SIGN_LOSS,
casts::CAST_SLICE_DIFFERENT_SIZES,
casts::CHAR_LIT_AS_U8,
casts::FN_TO_NUMERIC_CAST,
casts::FN_TO_NUMERIC_CAST_ANY,
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ msrv_aliases! {
1,46,0 { CONST_IF_MATCH }
1,45,0 { STR_STRIP_PREFIX }
1,43,0 { LOG2_10, LOG10_2 }
1,42,0 { MATCHES_MACRO, SLICE_PATTERNS }
1,42,0 { MATCHES_MACRO, SLICE_PATTERNS, PTR_SLICE_RAW_PARTS }
1,41,0 { RE_REBALANCING_COHERENCE, RESULT_MAP_OR_ELSE }
1,40,0 { MEM_TAKE, NON_EXHAUSTIVE, OPTION_AS_DEREF }
1,38,0 { POINTER_CAST }
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/cast_slice_different_sizes.rs
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];
}
52 changes: 52 additions & 0 deletions tests/ui/cast_slice_different_sizes.stderr
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

4 changes: 2 additions & 2 deletions tests/ui/transmutes_expressible_as_ptr_casts.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ fn main() {
let slice_ptr = &[0, 1, 2, 3] as *const [i32];

// ... or pointer_kind(T) = pointer_kind(U_0); ptr-ptr-cast
let _ptr_to_unsized_transmute = unsafe { slice_ptr as *const [u16] };
let _ptr_to_unsized = slice_ptr as *const [u16];
let _ptr_to_unsized_transmute = unsafe { slice_ptr as *const [u32] };
let _ptr_to_unsized = slice_ptr as *const [u32];
// TODO: We could try testing vtable casts here too, but maybe
// we should wait until std::raw::TraitObject is stabilized?

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/transmutes_expressible_as_ptr_casts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ fn main() {
let slice_ptr = &[0, 1, 2, 3] as *const [i32];

// ... or pointer_kind(T) = pointer_kind(U_0); ptr-ptr-cast
let _ptr_to_unsized_transmute = unsafe { transmute::<*const [i32], *const [u16]>(slice_ptr) };
let _ptr_to_unsized = slice_ptr as *const [u16];
let _ptr_to_unsized_transmute = unsafe { transmute::<*const [i32], *const [u32]>(slice_ptr) };
let _ptr_to_unsized = slice_ptr as *const [u32];
// TODO: We could try testing vtable casts here too, but maybe
// we should wait until std::raw::TraitObject is stabilized?

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/transmutes_expressible_as_ptr_casts.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ LL | let _ptr_i8_transmute = unsafe { transmute::<*const i32, *const i8>(ptr
error: transmute from a pointer to a pointer
--> $DIR/transmutes_expressible_as_ptr_casts.rs:28:46
|
LL | let _ptr_to_unsized_transmute = unsafe { transmute::<*const [i32], *const [u16]>(slice_ptr) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `slice_ptr as *const [u16]`
LL | let _ptr_to_unsized_transmute = unsafe { transmute::<*const [i32], *const [u32]>(slice_ptr) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `slice_ptr as *const [u32]`

error: transmute from `*const i32` to `usize` which could be expressed as a pointer cast instead
--> $DIR/transmutes_expressible_as_ptr_casts.rs:34:50
Expand Down