Auto merge of #8445 - asquared31415:slice_ptr_cast, r=llogiq

bors · bors · commit 0c483f69db97 · 2022-03-06T07:46:56.000Z
Llint for casting between raw slice pointers with different element sizes

This lint disallows using `as` to convert from a raw pointer to a slice (e.g. `*const [i32]`, `*mut [Foo]`) to any other raw pointer to a slice if the element types have different sizes.  When a raw slice pointer is cast, the data pointer and count metadata are preserved.  This means that when the size of the inner slice's element type changes, the total number of bytes pointed to by the count changes.  For example a `*const [i32]` with length 4 (four `i32` elements) is cast `as *const [u8]` the resulting pointer points to four `u8` elements at the same address, losing most of the data.  When the size *increases* the resulting pointer will point to *more* data, and accessing that data will be UB.

On its own, *producing* the pointer isn't actually a problem, but because any use of the pointer as a slice will either produce surprising behavior or cause UB I believe this is a correctness lint.  If the pointer is not intended to be used as a slice, the user should instead use any of a number of methods to produce just a data pointer including an `as` cast to a thin pointer (e.g. `p as *const i32`) or if the pointer is being created from a slice, the `as_ptr` method on slices.  Detecting the intended use of the pointer is outside the scope of this lint, but I believe this lint will also lead users to realize that a slice pointer is only for slices.

There is an exception to this lint when either of the slice element types are zero sized (e.g `*mut [()]`).  The total number of bytes pointed to by the slice with a zero sized element is zero.  In that case preserving the length metadata is likely intended as a workaround to get the length metadata of a slice pointer though a zero sized slice.

The lint does not forbid casting pointers to slices with the *same* element size as the cast was likely intended to reinterpret the data in the slice as some equivalently sized data and the resulting pointer will behave as intended.

---

changelog: Added ``[`cast_slice_different_sizes`]``, a lint that disallows using `as`-casts to convert between raw pointers to slices when the elements have different sizes.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3077,6 +3077,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
diff --git a/clippy_lints/src/casts/cast_slice_different_sizes.rs b/clippy_lints/src/casts/cast_slice_different_sizes.rs
@@ -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
+    }
+}
diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs
@@ -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;
@@ -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,
+    "casting using `as` between raw pointers to slices of types with different sizes"
+}
+
 pub struct Casts {
     msrv: Option<RustcVersion>,
 }
@@ -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,
@@ -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);
diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs
@@ -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),
diff --git a/clippy_lints/src/lib.register_correctness.rs b/clippy_lints/src/lib.register_correctness.rs
@@ -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),
diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -78,6 +78,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,
diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs
@@ -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 }
diff --git a/tests/ui/cast_slice_different_sizes.rs b/tests/ui/cast_slice_different_sizes.rs
@@ -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];
+}
diff --git a/tests/ui/cast_slice_different_sizes.stderr b/tests/ui/cast_slice_different_sizes.stderr
@@ -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
+
diff --git a/tests/ui/transmutes_expressible_as_ptr_casts.fixed b/tests/ui/transmutes_expressible_as_ptr_casts.fixed
@@ -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?
 
diff --git a/tests/ui/transmutes_expressible_as_ptr_casts.rs b/tests/ui/transmutes_expressible_as_ptr_casts.rs
@@ -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?
 
diff --git a/tests/ui/transmutes_expressible_as_ptr_casts.stderr b/tests/ui/transmutes_expressible_as_ptr_casts.stderr
@@ -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
